Skip to content

fix(knowledge): refresh unchanged mirrored ACLs without firing the projection fan-out - #8169

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/drive-admin-sync-statement-timeout
Sep 22, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/drive-admin-sync-statement-timeout

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Admin-mode (mirrored ACL) syncs rewrote acl on every listed document every page, even when unchanged. The document trigger copies acl onto every chunk's search projection rows, and a projection row the backfill hasn't filled yet (acl IS NULL) is distinct from every ACL — so each unchanged document still rewrote all its chunks, each one a vector index insert. One ACL batch could outrun the 60 s statement timeout (SQLSTATE 57014) and fail the whole sync right after the permission fetch
  • persistDocumentAcls now splits each ACL group into two statements: rows whose acl and acl_requirements already match only get acl_verified_at refreshed (no acl in the SET list, so the trigger never fires); only rows that actually differ get the ACL write
  • Changed ACL writes go in batches of 25 instead of 500, since their cost scales with the chunks of every document in the statement, not the document count
  • Evidence semantics are unchanged: unchanged ACLs still get a fresh acl_verified_at, the unresolved-duplicate guard applies to both statements, and a row is counted once (refresh runs first)

Type of Change

  • Bug fix

Testing

  • New sync-persistence.postgres.test.ts against real Postgres with the projection trigger installed: an unchanged ACL leaves unfilled projection rows untouched and refreshes evidence; a changed ACL still propagates to filled and unfilled rows; a changed restriction under the same primary ACL counts as a change; a mixed page writes each document once
  • Mutation-checked: reverting the fix fails 2/4, dropping the restriction comparison fails 1/4, dropping the changed-row guard fails 2/4, swapping statement order fails 2/4
  • Updated mocked sync-persistence.test.ts; lib/knowledge/access, lib/knowledge/connectors, connectors/google-drive suites pass with Postgres (1206 tests)
  • bun run lint, bun run check:audits (47), docs-manifest:check, type-check 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)

…ojection fan-out

An admin-mode sync rewrote the ACL of every listed document on every page, even
when it had not changed. Assigning `acl` fires the document trigger that copies
it onto every chunk's search projection rows, and a projection row the backfill
has not filled yet is distinct from every ACL, so each unchanged document still
rewrote all of its chunks, each one a vector index insert. A single ACL batch
could then exceed the statement timeout and fail the whole sync.

Unchanged ACLs now only refresh their evidence timestamp, which never assigns
`acl`. Documents whose ACL or restrictions actually changed are written in
smaller batches, since each one fans out to all of its chunks.
@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:29pm UTC

Request Review

@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

@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 follow-up changes add effective regression coverage without introducing a new actionable issue.

Summary

This PR avoids projection fan-out when mirrored document ACLs have not changed while preserving evidence refresh semantics.

  • Separates unchanged-row evidence refreshes from changed ACL writes.
  • Reduces changed-ACL batches to account for per-document chunk projection work.
  • Adds real-PostgreSQL coverage for unchanged, changed, restricted, mixed, batched, and unresolved ACL cases.
  • Adds the PostgreSQL regression suite to CI.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Group documents by desired ACL state] --> B{Stored ACL and requirements match?}
  B -->|Yes| C[Refresh acl_verified_at only]
  B -->|No| D[Write ACL, requirements, and evidence]
  C --> E[No projection trigger fan-out]
  D --> F[Document trigger updates chunk projections]
  E --> G[Count updated document once]
  F --> G
Loading

Reviews (2) · Last reviewed commit: "test(knowledge): run the ACL write Postg..."

@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 4 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 e29f04c into staging Sep 22, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/drive-admin-sync-statement-timeout branch September 23, 2026 00:05

This branch was successfully deployed

1 active deployment
Preview 5e2a2067 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