Skip to content

fix(knowledge): write external group membership as a diff - #7943

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/external-group-membership-diff
Sep 17, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/external-group-membership-diff

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Every directory sync deleted a group's entire external membership and reinserted it, whether or not anything had changed. That took the membership table to ~55M lifetime inserts and ~55M deletes against ~127k live rows — about 430x write amplification — holding it near 91% dead tuples across 2,447 autovacuum cycles, an order of magnitude more churn than any comparable table. That vacuum load competes for the same I/O as every other query on the instance.
  • Membership is now written as a difference: read the group's current tokens, delete only what the enumeration no longer lists, insert only what it newly lists. An unchanged group writes nothing.
  • The group row is locked before that read. The two callers fence on different leases — the directory lease and the connector sync lease — so neither excludes the other, and on the directory path the group upsert commits in a separate transaction, so its row lock is already released. Without the lock the removal set is computed against a snapshot a concurrent pass may have moved past, and a subject that pass inserted would survive an enumeration that never observed it: membership retained rather than revoked. The lock also closes the narrower window the blind delete already had between its delete and its commit.
  • created_at on a member row now means first-observed rather than last-observed. No reader projects it, and the group's lastSyncedAt remains the freshness signal and is still written every pass.
  • Also corrects the processing-queue TSDoc, which argued the per-tenant lane limits were sized rather than inherited, and cited a concurrency figure derived from an unsound measurement.

Type of Change

  • Bug fix

Testing

  • 3,150 tests pass across lib/knowledge and background; one pre-existing failure that needs DATABASE_URL
  • bun run type-check clean, bun run lint clean, all 46 audits pass, docs manifest in sync
  • Four tests cover it — unchanged group writes nothing, changed group writes only the delta, the group row is locked before the read, and the partial-sync case still writes the accessible group — each verified to fail when the fix is reverted
  • Concurrency reasoning is from the lease and transaction structure, not an executed concurrent test; a Postgres integration test running two passes against one group would be the way to prove it empirically

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Every directory sync deleted a group's entire membership and reinserted
it, whether or not anything had changed. That took the membership table
to roughly 55M lifetime inserts and 55M deletes against ~127k live rows
— about 430x write amplification — holding it near 91% dead tuples
across 2,447 autovacuum cycles, an order of magnitude more churn than
any comparable table. The vacuum load that generates competes for the
same I/O as every other query on the instance.

Membership is now written as a difference: read the group's current
tokens, delete only those the enumeration no longer lists, insert only
those it newly lists. An unchanged group writes nothing.

The group row is locked before that read. The two callers fence on
different leases — the directory lease and the connector sync lease — so
neither excludes the other, and on the directory path the group upsert
commits in a separate transaction, so its row lock is already released.
Without the lock, the removal set is computed against a snapshot a
concurrent pass may have moved past, and a subject that pass inserted
would survive an enumeration that never observed it: membership retained
rather than revoked. Locking also closes the narrower window the blind
delete already had between its delete and its commit.

`created_at` on a member row now means first-observed rather than
last-observed. No reader projects it, and the group's `lastSyncedAt`
remains the freshness signal and is still written every pass.

Also corrects the processing-queue TSDoc, which argued the per-tenant
lane limits were sized rather than inherited and cited a concurrency
figure derived from an unsound measurement.
@vercel

vercel Bot commented Sep 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Sep 17, 2026 9:00pm UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the previous lock-order coverage concern is fully addressed and no new actionable issues were identified.

Summary

This PR reduces external-group membership write amplification by replacing full membership rewrites with serialized set-difference updates.

  • Locks each external group before reading its current membership.
  • Deletes and inserts only changed membership tokens while preserving unchanged rows.
  • Adds coverage for unchanged, changed, partial-sync, and lock-order behavior.
  • Corrects documentation for processing-queue concurrency limits.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Enumerate external group members] --> B[Validate member tokens]
    B --> C[Lock external group row]
    C --> D[Read existing membership]
    D --> E[Compute desired minus retained]
    D --> F[Compute existing minus desired]
    E --> G[Insert added members]
    F --> H[Delete removed members]
    G --> I[Update lastSyncedAt]
    H --> I
    I --> J[Commit transaction]
Loading

Reviews (2) · Last reviewed commit: "test(knowledge): assert the group lock p..."

Comment thread apps/sim/lib/knowledge/connectors/external-group-sync.test.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

Presence of the lock was the only thing asserted, so moving it below the
read would have kept the test green while reopening the stale-snapshot
race the lock exists to close. The assertion is now the relative call
order of the lock and the member-table read.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit bfaeaba into staging Sep 17, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/external-group-membership-diff branch September 17, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant