feat(data-export): add organization data exports - #5212
Conversation
Adds a second export subject alongside the existing personal one. An organization export selects on organization_id, so it covers work members did in the organization and excludes their personal activity, and reaches org-owned rows that carry no user at all. Selection never enumerates members: the organization tag is on the row, so a former member's org work is included and a current member's personal work is not. Membership decides only whether the requester may ask. Unique per organization rather than per requester, authorised at request and again at download, with the Worker re-checking membership independently.
Resolves against the two-step emailed-code download from #5204: the organization branch moves into requireDownloadableExport, the shared gate for both download steps, so neither can be reached on stale authority. Migration renumbered to 0213 behind main's 0212.
not loaded yet
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental commit fixes the silent row-loss ordering defect in the two composite-cursor export queries with no new findings; one previously-acknowledged migration SUGGESTION remains open. Overview
The incremental change table-qualifies the ORDER BY cursor columns in the Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 986d491)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 986d491)Status: 1 Issue Found | Recommendation: Address before merge Overview
The incremental commit fixes the previous WARNING: Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 89b99f3)Status: 2 Issues Found | Recommendation: Address before merge Overview
The organization-export feature is well structured: a single shared access predicate ( Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (17 files)
Reviewed by kimi-k3 · Input: 57.9K · Output: 5.3K · Cached: 356.4K Review guidance: REVIEW.md from base branch |
…ares `ORDER BY most_significant_position` bound to the SELECT list's `most_significant_position::text AS most_significant_position` rather than to the bigint column, because a bare name in ORDER BY resolves to a matching output column before an input column. The cursor tuple in the WHERE clause cannot see output aliases, so it kept comparing the bigint. Two silent failures followed. The page was ordered lexicographically while the cursor advanced numerically, so a page's last row was not its cursor maximum and the next page's `>` skipped whatever the text order had deferred -- the row loss the composite cursors were built to prevent. Reproduced on Postgres with positions (10,9) (10,10) (10,100): page 1 returned (10,10) and (10,100), and (10,9) was then excluded from every later page. And no btree can serve a text ordering of a bigint column, so each page bitmap-scanned and sorted the whole owner's rowset: 116,522 startup cost per page against 0.42 once qualified, measured on a 115k-row organization. Table-qualifying the shadowed name makes it an input reference again, which is what both the index and the cursor comparison are built on. The COALESCE key needs no qualifier -- an expression already binds to the input column. Guarded for the whole set rather than these two queries: no column selected as `X::type AS X` may appear as a bare name in that query's ORDER BY.
2 participants
Summary
Adds an organization data export alongside the existing personal one. An
organization export selects rows by
organization_id, so it covers work membersdid in the organization's context plus the rows the organization owns outright,
and excludes members' personal activity. Selection never enumerates members. The
organization tag is on the row itself, so a former member's organization work is
included and a current member's personal work is not. Membership only decides
whether the requester may ask.
The branch also carries three changes to the existing personal export.
Export subject
user_data_exportsgainssubject_typeandorganization_id, with a CHECKconstraint tying them together and partial unique indexes giving one active
export per user and one per organization. Two admins pressing the button share
one job rather than generating duplicate copies.
its query. The predicate allow list gained
organization_id = $1, and a testpins each query's scope to what its name claims.
subjectTypeandorganizationId, so aconsumer does not have to infer which kind of export it is holding.
organization the signed in person may export. History rows gained a badge
showing whether a row is personal or an organization's.
Fixes ordering that silently dropped rows
Both journal-style queries cast a bigint to text and aliased it back to the
column's own name, for example
most_significant_position::text AS most_significant_position.ORDER BYresolves a bare identifier to an outputcolumn in preference to a table column, so the pages were ordered
lexicographically. The
WHEREclause cannot see output names, so it keptcomparing the raw bigints.
Keyset paging requires the ordering and the cursor predicate to agree. They did
not, so the cursor was taken from a row that was not the numerically largest,
and later pages both repeated rows and skipped others permanently. Text and
numeric order diverge wherever digit counts differ, and
least_significant_positioncounts up from small numbers.Confirmed on real query plans, which showed a Sort on the text cast sitting
above indexes that already cover the intended ordering:
The casts keep their text output, since bigint precision has to survive JSON.
Only the aliases are renamed, so
ORDER BYresolves to the columns. Re-measuredafter the change, both are bare index scans with no sort. A structural test
enforces that an output alias never reuses the name of a column the
ORDER BYmentions.
This affects the personal export that is already shipped, not only the
organization work.
Fixes a row loss defect in the personal export
system_prompt_prefixwas paginated onsystem_prompt_prefix_idalone, but thattable's grain is the triple of prefix id, user and organization, so the id
repeats. Verified against the warehouse: one user carries prefix id 2 twice, once
with an organization and once without, and one organization carries prefix id 1
nine times across nine users. A single column keyset cursor pages with
id > cursor, so any duplicate landing on a page boundary was dropped silently.The cursor is now composite, and both variants were checked against real rows.
Deleted rows are labelled rather than dropped
The warehouse retains rows prod has deleted. Records from the three tables
carrying
_snowflake_deletednow getsoftDeleted: truewhen the row wasdeleted. Nothing is filtered out, since the export is meant to be a truthful copy
of what is held. Only a positive deletion is reported, because a live row and a
row whose state is not yet known are indistinguishable until a table's reload
runs. Project and prompt records gained an
idso the label has something toattach to, matching the other sources which already emitted one.
Tolerates warehouse tables that have not loaded
The warehouse is loaded table by table and this ships ahead of it. Previously a
missing table threw, was treated as retryable, burned four queue retries and dead
lettered the whole export. A single
information_schemaprobe now runs beforeanything is written, and the header names what was set aside in
unavailableSourcesnext toincludedSources. The probe checks columns ratherthan just table existence, because a table loaded earlier can lack a column a
newer query selects.
Verification
Every query the Worker issues was run against the real snowflake-exports
database, for one user and one organization, checking both returned content and
query plans.
Personal export, all seven queries: availability probe, identity, and the
five sources. Two bugs found (see the ordering section above), five passed.
Organization export, all five sources in the order the Worker runs them,
with the same page sizes.
Attribution cross-checked against production. The user owns no App Builder
projects and created two that the organization owns, which is why his
personal export correctly returns none.
App Builder attribution proven by set comparison rather than sampling.
Messages tagged to the organization whose project is not the
organization's: 0. Messages of the organization's projects not tagged to
it: 0. Both counts 41,967, so the two sets are identical.
Ordering fix re-measured. Both queries are bare index scans with no sort.
Availability probe verified. All six tables carry every column their
source requires, so
unavailableSourcesis empty.Request and download an export end to end through the UI.
Visual Changes
New "Organization exports" card, shown only to people who own or administer at
least one organization. History rows gained a personal or organization badge.
Screenshots still needed.
Reviewer Notes
Authorization deliberately does not use
ensureOrganizationAccess. Thathelper grants
ownerto anyis_admincaller, and every procedure on thisrouter is
adminProcedure, so using it would authorize on Kilo staff statusrather than on organization membership. A Kilo employee with
/adminaccess isnot a member of a customer's organization and cannot export it. The rule lives in
packages/db/src/organization-export-access.tsand is shared by the router andthe Worker, which re-checks independently. When the two carried separate copies,
the Worker's lacked parent organization inheritance, and an export generated,
showed as ready, then refused every download while holding the organization's
only active export slot.
Migration touches a live table.
0213drops and rebuilds a partial uniqueindex on
user_data_exportsand adds two CHECK constraints withoutNOT VALID, all in one transaction. Both constraints are satisfied by everyexisting row, since
subject_typedefaults touserandorganization_idisnull, so this is lock duration rather than a risk of failing. Worth a count on
that table before merging if it is larger than expected.
Organization exports run the same 13 minute one shot generator as personal
ones, with no persisted cursor. A sufficiently large organization can exhaust
that budget and fail terminally with no partial progress retry.
The re-request throttle is still the temporary 5 minute value and predates
this branch, but organization exports now share that window while the message
still says 24 hours. Restoring it is one edit, since both paths share
createExportRequest.microdollar_usage_metadatahas no organization index. Its organizationquery currently plans as a parallel sequential scan over 315 GB, and the Worker
pages, so each page would repeat that scan. An index on
(organization_id, id)is being built. Organization exports should not be enabled for that source until
it lands. The other four organization sources are all served by existing indexes
with no sort.