Skip to content

#18 follow-ups: sync-state lock, status pending-count, sources pin test, E2 CI matrix (one PR) #59

Description

@YellowSnnowmann

Follow-up to #18, consolidating every remaining finding from its end-to-end review into one issue → one PR. Each item is real, verified against the code, and independent of the #18 acceptance criteria (which are all met by #48). None is a regression introduced by #18 — three are pre-existing defects the review surfaced, two are hardening the review recommended.

Fix all of these together in a single PR; check items off as they land.

1. SyncState has no per-connection lock — concurrent syncs clobber cursor/dedup/budget

(was #50 — pre-existing engine design, ported faithfully by #48)

run_incremental_sync (core/src/sync/pipelines/composio/orchestrator.rs) loads SyncState from KV, mutates in memory for the whole run, saves at the end. No lock, no version check. The periodic loop and a trigger/RPC sync for the same connection race: whichever saves last wins, losing either the dedup set (→ re-fetch, re-spend) or the daily budget count (→ overspend past the cap).

Fix: a per-(toolkit, connection_id) async mutex held for the run — held in sync::pipelines::host (single-process, matches how the periodic loop and RPC paths actually run). An optimistic version stamp on the KV record is the multi-process answer if that ever matters; not needed today.

2. sources/status.rs counts chunks_pending via the dead legacy embedding column

(was #51 — pre-existing, untouched by any #18 PR)

SUM(CASE WHEN embedding IS NULL …) over mem_tree_chunks — the current engine never writes that column, so every source shows chunks_pending == chunks_synced forever. The memory-sources UI shows eternal pending on healthy sources.

Fix: read pending from the live embedding-state table. And this file is the third copy of the FreshnessLabel/status family (engine sync/status.rs, core sync/sync_status, this) — consolidate onto core::sync::sync_status while here, which also closes item 4.

3. Serde pin test for the tinymemory-sources types twin

(review finding, not yet filed)

sources/src/types.rs and the engine's memory/sources/types.rs are identical today and share a live serde wire: core/src/engine/sync.rs converts between them via serde_json::to_value/from_value for the tree-coupled source kinds. Every other deliberate twin in the arc has a pin test; this one has none. If tinycortex adds a SourceKind variant or renames a field, the next pin bump fails at runtime on the first external-source sync.

Fix: a source_entry_wire_format_is_pinned test in sources/src/types.rs asserting the full serialised shape of a populated MemorySourceEntry (same pattern as state_line_format_is_pinned), so drift becomes a CI failure.

4. Third FreshnessLabel copy

Covered by item 2 — listed so it is not forgotten if item 2 is scoped down.

5. CI tests fewer feature configurations than issue #18 §E2 names

(review finding, not yet filed)

§E2 lists nine build+test configs (--no-default-features · tinycortex · tinycortex,memory-git · tinycortex,contacts on macOS · mem0 · supermemory · cognee · sync-composio · --all-features). .github/workflows/ci.yml runs cargo test for --all-features and default only, plus cargo hack --feature-powerset --depth 2 … check — a check, not a test. A regression that only manifests at runtime under --features mem0 alone merges green.

Fix: add cargo test -p tinymemory --features <X> for each single-engine feature to the Feature-powerset job (or a matrix). The contacts-on-macOS row needs a macos-latest runner; acceptable to leave that one as check-only with a comment if runner minutes are a concern.

Notes

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions