Skip to content

fix(knowledge): stop document ACL changes from rewriting unfilled projection chunks - #8175

Merged
waleedlatif1 merged 3 commits into
stagingfrom
fix/projection-acl-trigger-skip-unfilled
Sep 22, 2026
Merged

waleedlatif1 merged 3 commits into
stagingfrom
fix/projection-acl-trigger-skip-unfilled

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • sync_projection_source_acl() updated every enabled chunk whose projected ACL differed from the document's; a chunk the projection backfill hasn't filled (acl IS NULL) always differs, so any real document ACL change rewrote all its unfilled chunks (each a vector index insert) inside the writer's statement — admin ACL writes, the permissions-only revoke, and rewriteConnectorAcls all paid it
  • New trigger body: unfilled chunks keep acl = NULL; filled chunks still get the new ACL and source; unfilled chunks get the new source only when the document actually moves (OLD.connector_id IS DISTINCT FROM NEW.connector_id), since source filters read connector_id off the row
  • Ships as script migration 0023_projection_acl_skip_unfilled (runs in the migrate job after SQL migrations), which only CREATE OR REPLACEs the function; 0022/db:push install the same body via the shared replaceProjectionSourceAclSync
  • Adds a CI step for the projection trigger/backfill Postgres test, which was not run in CI before

Why it's safe

  • Search decides a NULL-acl projection row on its document (projectionCandidateAccessCondition unfilled branch), so leaving it NULL can't leak or hide anything; the "projection filled" probe only ever sees fewer NULLs, never new ones
  • Backfill pages read connector_id/acl from the document FOR SHARE and only write rows still NULL. Change committed first → the page reads the new values. Page holds the share lock first → the change waits, then its trigger sees a filled row and overwrites it. Change uncommitted when the page arrives → the page waits and rechecks the updated document. All three are covered by tests with real lock waits
  • The projection BEFORE INSERT/UPDATE OF document_id, enabled trigger is unchanged, so new and re-enabled chunks are still filled from their document

Type of Change

  • Bug fix

Testing

  • Postgres tests: changed ACL updates filled chunks and skips unfilled ones; a moved document still carries its source onto unfilled chunks; 0023 replaces the previous body in place; three backfill/ACL-change interleavings end with the current ACL
  • Mutation-checked: old body → 3 red; skipping unfilled rows entirely → 1 red (source move); writing the ACL onto unfilled rows on a move → 1 red; syncing source onto unfilled rows without a move → 1 red; 0023 as a no-op → 1 red
  • bun run lint, bun run check:audits, type-check (packages/db, apps/sim), check:migrations origin/staging pass

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)

…jection chunks

The document trigger copied a changed ACL onto every enabled chunk whose
projected ACL differed, and a chunk the projection backfill has not filled yet
(acl IS NULL) always differs. Each such write re-inserts the row into the vector
index, so one document ACL change inside a sync or member rewrite could rewrite
every unfilled chunk of that document in the writer's own statement.

Unfilled chunks now keep a NULL ACL: search already decides them on their
document, and the backfill copies the document's current values under a share
lock. A document that moves to another source still carries the source onto
them, since source filters read it from the row. The new body ships as script
migration 0023, which replaces the function in place; 0022 installs the same
body on a fresh database.
@vercel

vercel Bot commented Sep 22, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated
docs Ready Ready Preview Sep 22, 2026 10:47pm 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 prior CI prerequisite issue is resolved and no new actionable defects remain.

Summary

This PR changes the projection ACL synchronization function so document ACL updates avoid rewriting projection chunks whose ACL backfill has not completed, while connector moves still update their source metadata.

  • Adds an ordered script migration that replaces the trigger function for existing databases.
  • Keeps new installations and schema-push environments aligned through the shared function installer.
  • Adds PostgreSQL coverage for filled and unfilled chunks, connector moves, migration replacement, and concurrent backfill/ACL-update interleavings.
  • Adds the PostgreSQL integration test to CI with self-contained test-database setup.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  D[Document ACL or connector changes] --> T[sync_projection_source_acl trigger]
  T --> F{Projection ACL filled?}
  F -->|Yes| U[Update connector source and ACL]
  F -->|No| M{Connector moved?}
  M -->|Yes| S[Update connector source; preserve NULL ACL]
  M -->|No| N[Leave projection row untouched]
  S --> B[Backfill later copies current document ACL]
  N --> B
Loading

Reviews (2) · Last reviewed commit: "chore(ci): create the ACL test database ..."

Comment thread .github/workflows/test-build.yml

@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 6 files

Confidence score: 5/5

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

Re-trigger cubic

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

@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 7 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 aaca125 into staging Sep 22, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/projection-acl-trigger-skip-unfilled branch September 22, 2026 22:55

This branch was successfully deployed

1 active deployment
Preview 8906c421 Deployed Sep 22, 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