Skip to content

fix(knowledge): bound the members-mode tombstone pass by what a run changed - #8172

Merged
waleedlatif1 merged 11 commits into
stagingfrom
fix/member-lifecycle-bounded-tombstone
Sep 23, 2026
Merged

waleedlatif1 merged 11 commits into
stagingfrom
fix/member-lifecycle-bounded-tombstone

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The members-mode lifecycle's tombstone phase ran one documentNOT EXISTS (observation) anti-join over every live document of the connector on every run once any member had completed a listing. Almost every document is observed, so LIMIT 500 never stopped the walk. A member run died at the statement timeout after applyMemberListing had already committed that member's page and observations. The run then failed and pushed the connector down the failure ladder. This statement is inferred as the one that timed out, not observed directly: it is the only statement in the run whose cost grows with the connector's live documents. Locally the planner chose a Parallel Seq Scan with a Hash Anti Join for it, not the nested-loop plan seen in production
  • Tombstoning now starts from the documents whose observations this run removed: unseen after a complete listing, withdrawn by a change feed, or held by a member being removed. Each batch is a UPDATE ... WHERE id IN (<=500 ids) AND NOT EXISTS (...), a nested-loop anti join bounded by the batch These are tombstoned even when no member with a completed listing remains, as the stale-member sweep already does. Previously, removing the only listed member left the documents only it observed live forever. The backstop and the purge stay gated on a completed listing
  • Absence the run did not cause (a member deleted by an earlier run, a document un-excluded, a connector whose first listing just finished, a run that stopped between removing observations and tombstoning) goes to a resumable backstop. It walks the connector's live documents by external id through the existing doc_connector_external_id_idx, one page per statement with LIMIT on the document side. It checks observations only in the UPDATE over that page's ids, and saves its position in a new nullable knowledge_connector.member_tombstone_cursor. External id is the key because it never changes, while source_seen_at is rewritten by every listing and a walk ordered by it would never finish a pass. Each run does at most MEMBER_TOMBSTONE_RECONCILE_PAGES_PER_RUN (20) pages, so a pass completes within ceil(documents / budget) runs. A small connector still finishes a full pass every run
  • Which documents get tombstoned, resurrected, or purged does not change. What changes is when: a document that goes unobserved without this run removing its observation may be tombstoned a few runs later. Its ACL is already empty by then, so this changes nothing about who can read it, only when purge starts counting
  • Resurrection and purge are unchanged. Their scans already go through doc_connector_tombstone_idx, so they are bounded by tombstones
  • Removing a member now tombstones, page by page, the documents no other member observes. This happens in the same transaction that advances the removal's checkpoint. It does not depend on the run reaching its lifecycle, which matters when the removed member was the last one with a completed listing and the run stops right after the deletion. A connector whose corpus is owned by a dedicated content credential is left alone, as before. The backstop writes its cursor once per run. Connector presenters drop member_tombstone_cursor along with the stored API key, so it never reaches API responses. member-document-lifecycle.integration.ts now runs in CI with the other knowledge PostgreSQL suites
  • Still left to the backstop, and gated on a completed listing as before: observations removed by credential deletion cascading member rows, a GitHub repository identity change deleting every member, and documents restored from exclusion
  • Migration 0377 (on top of fix(knowledge): release kept connector documents in the background so removing a large source cannot time out #8165's 0376): ADD COLUMN member_tombstone_cursor jsonb, nullable and with no default, so it is metadata-only and older app versions are unaffected. No index changes

Type of Change

  • Bug fix

Testing

  • Unit (member-observations.test.ts): a removed-this-run id is tombstoned through a batch UPDATE that rechecks NOT EXISTS and skips already-tombstoned rows; the page SELECT has no observation filter and carries LIMIT 500; the page budget stops the walk and saves the cursor; a later run starts from the saved cursor; the deadline leaves work for the next run
  • Engine (member-sync-engine.integration.test.ts): ids from complete-listing removal and change-feed withdrawal reach the lifecycle
  • Postgres (member-document-lifecycle.integration.ts, new case): with more unobserved documents than one run's budget, the document this run unobserved is tombstoned right away even though it sits past the budget. A document another member still observes is not tombstoned. The rest is finished on the next run and the cursor resets. listing-continuation.integration.ts now asserts that removing a member hands over its remaining observed documents. All existing member integration suites pass (jira/github/gmail/calendar/drive-shortcuts/excluded/listing-continuation/scope-renewal/directory-sync) Two more cases: removing the only member with a completed listing still tombstones what only it observed, but not a document another member observes or one nobody ever observed. A connector larger than the budget, with every observed document re-stamped between runs, still finishes a pass within ceil(N/budget) runs and reaches a document that lost its observers behind the cursor on the next pass
  • Mutation-checked each part by reverting it with file copies: the targeted phase, the page's observation filter, the page budget, the cursor load, the UPDATE recheck, and all three id collection points in the engine. Each revert turns the matching tests red
  • EXPLAIN on a synthetic local connector: the old shape (a nested loop anti join over the reconciliation index) visited every live document to return 1 row. Each new page is an Index Scan on doc_connector_external_id_idx under Limit, reading about 18 buffers for 500 rows at both the first page and a mid-connector cursor. The page UPDATE is a nested loop anti join over the PK, bounded to 500 probes
  • bun run lint, bun run check:audits (47 passed), bun run check:migrations origin/staging, bun run type-check (apps/sim)

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)

…hanged

The tombstone phase of the member document lifecycle ran one anti-join over
every live document of the connector on every run, and LIMIT could not bound
it because almost every document is observed. On a large connector that
statement outgrew the statement timeout, rolled back the member's page, and
walked the connector toward disabled.

Tombstoning is now driven by the documents whose observations the run itself
removed (unseen after a complete listing, withdrawn by a change feed, or held
by a member being removed), then by a resumable backstop that walks the
reconciliation index one page per statement and checks observations only in
the UPDATE over that page's live ids. The backstop saves its position on the
connector and does a bounded number of pages per run. Resurrection and purge
are unchanged.
@vercel

vercel Bot commented Sep 22, 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 23, 2026 12:04am UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the previously reported deadline handoff defect is fixed by committing each removal page’s tombstones with its checkpoint, and no new actionable issue remains.

Summary

This PR bounds members-mode document tombstoning by combining immediate, targeted handling of observations removed by the current run with a resumable connector-wide reconciliation pass.

  • Tombstones documents affected by completed listings, change-feed withdrawals, and member removals without scanning the entire connector in one statement.
  • Persists reconciliation progress through member_tombstone_cursor and limits each run to a fixed page budget.
  • Makes member-removal tombstoning atomic with ACL rewrite progress and restores eligible documents when a removal is withdrawn.
  • Prevents the new cursor and encrypted connector credentials from reaching connector API responses.
  • Adds PostgreSQL, engine, unit, migration, and CI coverage for lifecycle continuation, deadline, access, and pagination behavior.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Run[Member sync run] --> Removed{Observations removed?}
  Removed -->|Yes| Targeted[Targeted tombstone batches]
  Removed -->|No| Eligible
  Targeted --> Eligible{Completed listing exists?}
  Eligible -->|Yes| Load[Load member tombstone cursor]
  Eligible -->|No| Resurrection[Resurrect eligible observed documents]
  Load --> Page[Read bounded live-document page by external ID]
  Page --> Recheck[UPDATE page IDs with NOT EXISTS observation recheck]
  Recheck --> Budget{Page budget or end reached?}
  Budget -->|More budget| Page
  Budget -->|Budget exhausted| Save[Save cursor for next run]
  Budget -->|End reached| Reset[Reset cursor]
  Save --> Resurrection
  Reset --> Resurrection
  Resurrection --> Purge[Purge eligible aged tombstones]
  Run --> Removal[Membership removal rewrite]
  Removal --> ACL[Rewrite one observation page's ACLs]
  ACL --> Sole[Tombstone documents observed only by removed member]
  Sole --> Checkpoint[Advance removal checkpoint atomically]
Loading

Reviews (3) · Last reviewed commit: "test(knowledge): insert the content-less..."

Comment thread apps/sim/lib/knowledge/connectors/member-sync-engine.ts
Comment thread packages/db/migrations/0377_member_tombstone_cursor.sql Outdated

@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.

All reported issues were addressed across 12 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/knowledge/connectors/member-observations.ts Outdated
…mbstone explicit removals without a completed listing

The absence reconcile ordered its resumable walk by source_seen_at, which
every member listing rewrites for what it observed. On a connector larger than
one run's page budget the walk chased re-stamped documents and a pass never
ended, so a document that lost its observers behind the cursor was never
revisited. The walk now keys on external id through
doc_connector_external_id_idx, which a document never changes, so a pass
completes within ceil(documents / budget) runs.

Documents a run explicitly unobserved (a complete listing, a change-feed
withdrawal, or a member removal) are now tombstoned even when no member with a
completed listing remains, as the stale-member sweep already does. Removing
the only listed member previously left every document only it observed live
and unobserved indefinitely. The reconcile and the purge stay gated on a
completed listing.
…ide the reconcile cursor

- A member removal interrupted by the run deadline resumed from its saved
  document cursor and only handed the lifecycle the documents above it. The
  resumed walk now re-reads the removed member's observations below the
  cursor first. They still exist until the member row is deleted.
- The absence reconcile writes its cursor once per run instead of once per
  page, and documents how many runs a pass takes.
- Connector presenters drop member_tombstone_cursor alongside the stored API
  key, so the internal cursor never reaches API responses.
- CI runs member-document-lifecycle.integration.ts with the other knowledge
  PostgreSQL suites.
@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 22, 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.

Comment thread apps/sim/lib/knowledge/connectors/member-sync-engine.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.

All reported issues were addressed across 17 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/knowledge/connectors/member-sync-engine.ts
Comment thread apps/sim/lib/knowledge/connectors/member-observations.ts
…removal itself

A membership removal handed the documents its member alone observed to the
lifecycle through run memory. If the run stopped after the member row was
deleted, the ids were gone, the observations had cascaded away, and with no
member left with a completed listing the absence reconcile never ran. Those
documents stayed live indefinitely.

Each removal page now tombstones the documents no other member observes in
the same transaction that advances the removal's checkpoint, so nothing
depends on the run surviving to its lifecycle. This applies only where
observations decide existence; a corpus owned by a dedicated content
credential is left alone, as before. The in-memory re-collection added
earlier is removed.

The absence reconcile's external_id IS NOT NULL filter is documented. Every
writer that sets connector_id copies a required external id, so the filter
excludes nothing.
…ember's restored ACLs

A multi-run member removal tombstones, page by page, the documents only that
member observes. If directory re-listing withdrew the removal mid-walk, the
restarted walk restored the member's token to those ACLs, but the documents
stayed deleted until the lifecycle's resurrection phase ran. That phase runs
only after the member loop finishes before the deadline.

A membership walk that is not a removal now applies the lifecycle's own
resurrect predicate to each page, in the same transaction. The predicate is
shared through one helper and covers: connector-owned, not excluded, not
archived, content present, observed. Only where observations decide
existence.
@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 23, 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.

All reported issues were addressed across 16 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/knowledge/connectors/member-observations.ts
@waleedlatif1
waleedlatif1 merged commit 916a355 into staging Sep 23, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/member-lifecycle-bounded-tombstone branch September 23, 2026 01:41

This branch was previously deployed

1 inactive deployment
Preview 97a65195 Deployed Sep 23, 2026 by vercel[bot]
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