Persist approved cohorts and serve them frozen (#857) - #1066
Draft
atriaybagur wants to merge 6 commits into
Draft
Conversation
At project approval each trust now materialises its cohort ONCE and persists it as a file artefact (parquet + metadata per hub project UUID, on a dedicated data-access-api bind mount, atomic-rename writes, no TTL). The two row-level routes serve ONLY that frozen artefact keyed on the encrypted project id and ignore caller-supplied SQL (hash-compared and logged on drift). No writes to the OMOP Postgres anywhere — the read-only engine stays the only DB connection. Closes the #857 gaps and its audit comment: - researcher FL code can no longer execute arbitrary SQL under a project id (/cohort/dataframe used to run any SELECT it was handed); - the cohort cannot grow after approval, and every training round fetches byte-identical data (Flower re-fetches per round — live drift used to land mid-run); - the imaging status poll (~10s while a project page is open) reads the frozen accession set instead of re-running cohort SQL against live OMOP, giving a stable denominator; - the hub finally records what was approved: a cohort_snapshot_status row per (project, trust) with row count, query id/hash and the approved-count diff, logged as a WARNING when membership drifted between submission and approval. Mechanics: - data-access-api: services/cohort_snapshot.py file store; POST /cohort/snapshot (validate_query remains the authority; threshold enforced before anything is persisted; oversize refused, never truncated) and POST /cohort/snapshot/delete (the not-yet-called #997 teardown hook); row-level routes fail closed on a missing snapshot; a snapshot without accession_id serves an EMPTY accession list so tabular/OMOP-only projects no-op imaging cleanly. - flip-api: TaskType.PERSIST_COHORT queued + committed BEFORE CREATE_IMAGING (dispatch orders by created_at; the trust poller is sequential), Alembic revision for the enum + audit table, post-processing via snapshot_notifications.py. - trust-api: handle_persist_cohort forwards to /cohort/snapshot and returns the snapshot facts as the task result. - imaging-api: all accession-id fetches now send the HUB project id (the XNAT project's secondary_ID) — snapshots are keyed on it; the XNAT uuid never matched anything. 403 details from data-access are relayed so below-threshold and no-snapshot refusals stay distinguishable. - deploy: per-trust COHORT_SNAPSHOT_STORAGE_DIR bind mount (kit Host-local profile; pre-created host-user-owned by make up-trust so the non-root service can write), wired in dev/prod/test compose. - tests: snapshot store + route unit suites, end-to-end snapshot lifecycle in the data-access integration stack, hub task-ordering + drift tests. The Testcontainers fixtures now build=True — the stack used to reuse a stale image, so dependency changes (pyarrow here) silently never reached it. - docs: data-access-api README/CLAUDE/AGENTS, root CLAUDE/AGENTS security section, and security.rst — whose old claim that cached results 'expire in minutes' was never true (60-day query-keyed cache); it now describes the frozen-cohort model honestly. No backward compatibility kept (no deployments exist yet): the live-SQL serving path on the row-level routes is deleted outright, so a project without a snapshot is refused rather than falling back. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
github-actions
Bot
changed the title
atriaybagur
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Without a snapshot volume the k8s trust's store self-disables and the new fail-closed row-level serving refuses every project. Adds a per-release PVC (dataAccessApi.snapshots: size/class/accessMode, default 5Gi RWO to match replicas: 1), mounts it at /snapshots with COHORT_SNAPSHOT_DIR set, and gives the pod fsGroup 1000 so the image's non-root user can write a fresh volume — the chart otherwise avoids fsGroup, but this one is load-bearing. Disabling dataAccessApi.snapshots.enabled is deliberate refusal of all row-level data. helm lint + template verified. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
A pre-#857 project has no frozen cohort artefact and the trusts' row-level routes now refuse it outright; the flag's help says so instead of letting the reuse flow die downstream with a bare 403. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
GET /projects/{id}/cohort-snapshots returns each trust's cohort_snapshot_status
record (aggregates only), and the project status card shows an 'Approved cohort
(frozen at approval)' strip per trust: frozen record count + snapshot date, an
amber 'drifted — approved on N' badge when the frozen count differs from the
count the project was approved on (#857's surface-the-diff requirement), and a
'tabular — no imaging' chip when the frozen cohort has no accession_id column —
which also explains that project's empty imaging panel. No polling: the frozen
cohort only changes at re-approval. A trust missing from the list has not
reported its snapshot (task pending/failed), matching the fail-closed serving.
flip-ui: CohortSnapshotSummary is its own component with its own useSWRV so the
existing ProjectStatus spec's single-ref swrv mock stays scoped; covered by its
own Vitest spec (written first).
Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
/cohort/snapshot read through get_records' 60-day query cache: the statistics run at submission caches the identical SQL, so a RE-approval — the one event that must propagate OMOP-side removals/opt-outs into the artefact — would silently re-freeze the stale pre-removal frame. get_records grows a use_cache flag (default unchanged); snapshot creation passes False and the fresh read still updates the cache. Pinned by a poisoned-cache service test and the router-call assertion. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
3 tasks
The trust-internal key gates every /cohort route, but fl-client legitimately holds it (it reads the frozen cohort via get_dataframe and pulls imaging), so that key alone cannot separate reading the approved cohort from DEFINING it. Split the cohort router by privilege: the read routes keep the trust-internal gate; the snapshot create/delete routes additionally require cohort-admin — proof of possessing AES_KEY_BASE64, sent as its SHA-256 (never the key itself) and constant-time compared. trust-api and data-access-api hold the AES key; fl-client does not, so researcher training code can no longer rewrite or delete a project's frozen cohort. A valid trust-internal key without the proof gets 403. No new secret is minted or distributed — the gate reuses the existing AES-key possession boundary, so kit files, register_trust, and compose are unchanged. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #857.
What
At project approval, each trust now materialises its cohort once and persists it as a file artefact —
dataframe.parquet+meta.jsonper hub project UUID on a dedicated data-access-api bind mount (atomic-rename writes, no TTL). The two row-level routes (/cohort/dataframe,/cohort/accession-ids) serve only that frozen artefact, keyed on the encrypted hub project id, and ignore caller-supplied SQL (hash-compared against the frozen query and logged on drift). No database writes anywhere: the read-only OMOP engine remains data-access-api's only DB connection, and researcher SQL (still pinned to theomopschema on the routes that do execute it) cannot reach a filesystem.Why (#857 + its security-audit comment)
/cohort/dataframeused to execute any SELECT that researcher FL code passed toflip.get_dataframe(project_id, query)— the decrypted project id was only ever logged. The query is now advisory; the only row-level data obtainable under a project id is the cohort that was approved./cohort/accession-ids(full cohort SQL) each time; it now reads the frozen pointer set — stable denominator, no per-poll query.cohort_snapshot_statusrow per (project, trust) with row count, query id/hash and the approved-count diff; membership drift between submission and approval is surfaced as a WARNING, never silently adopted. Opt-outs/removals propagate at explicit re-approval (atomic artefact replacement) and via the/cohort/snapshot/deletehook ([Feature]: Explicit administrator purge of Trust imaging for a deleted project #997) — never mid-training.How
data-access-api:
services/cohort_snapshot.py(project-UUID-keyed file store; path-safe, atomic, fail-closed),POST /cohort/snapshot(threshold enforced before anything persists; oversize refused, never truncated;validate_queryremains the authority on the one query executed),POST /cohort/snapshot/delete; row-level routes refuse a project with no snapshot; a snapshot withoutaccession_idserves an empty accession list, so tabular/OMOP-only projects no-op imaging cleanly.flip-api:
TaskType.PERSIST_COHORTqueued + committed beforeCREATE_IMAGING(dispatch orders bycreated_at, the trust poller is sequential), Alembic revision (enum value + audit table, drift guard green), post-processing inprivate_services/snapshot_notifications.py.trust-api:
handle_persist_cohortforwards to/cohort/snapshot; the snapshot facts ride back verbatim as the task result.imaging-api: all three accession-id fetch paths now send the hub project id (the XNAT project's
secondary_ID) — snapshots are keyed on it; the XNAT-minted uuid never matched anything. data-access 403 details are relayed so below-threshold and no-snapshot refusals stay distinguishable.deploy: per-trust
COHORT_SNAPSHOT_STORAGE_DIR(kit Host-local profile), bind-mounted as/snapshotsin dev/prod compose and pre-created host-user-owned bymake up-trust(docker would create it root-owned; the non-root service would then disable the store). Gitignored — it holds row-level patient data.Kubernetes chart: new per-release
cohort-snapshotsPVC (default 5Gi RWO,dataAccessApi.snapshots.*), mounted at/snapshotswith podfsGroup: 1000so the non-root image can write a fresh volume — without it the store self-disables and fail-closed serving refuses everything.helm lint+templateverified.No backward compatibility (per discussion — no deployments exist yet): the live-SQL serving path on the row-level routes is deleted outright.
e2e_smoke --project-idreuse of a pre-snapshot project is refused (documented in the flag's help) — re-approve or create a fresh project.Cohort-admin gate on the write routes: this PR adds a
POST /cohort/snapshotwrite route that defines the artefact everyone trains on. fl-client holds the sharedTRUST_INTERNAL_SERVICE_KEY(legitimately — it reads the frozen cohort and pulls imaging), so that key alone can't stop researcher code from rewriting/deleting the frozen cohort. The cohort router is split by privilege: read routes keep the trust-internal gate; the snapshot create/delete routes additionally require cohort-admin — proof of possessingAES_KEY_BASE64(sent as its SHA-256, constant-time compared), which trust-api + data-access-api hold and fl-client does not. No new secret is provisioned. The broader "give fl-client a least-privilege data-plane key" hardening is tracked in Least-privilege fl-client credential: two-tier trust-internal keys #1067.Hub + UI surfacing:
GET /projects/{id}/cohort-snapshotsreturns each trust's frozen-cohort record, and the project status card shows an "Approved cohort (frozen at approval)" strip per trust — frozen count + date, an amber drift badge when it differs from the approved count (Cohort membership is re-derived from live OMOP at every stage — no record of the approved cohort #857's surface-the-diff requirement), and a "tabular — no imaging" chip explaining an empty imaging panel.Cache-bypass fix: snapshot creation initially read through
get_records' 60-day query cache — a re-approval (the opt-out propagation event) would have silently re-frozen the stale frame the statistics run cached at submission.get_recordsnow takesuse_cache; snapshot creation passesFalse(pinned by a poisoned-cache test).Testing
RESULTS_UPLOADED): Flower ran the full lifecycle — snapshot artefact written at approval (/snapshots/<uuid>/dataframe.parquet+ meta), the ~10s status polls served "300 frozen accession ids" with zero OMOP queries, imaging pulled 599/600 from the frozen set, training fetched the frozen dataframe every round, and the hub recordedcohort_snapshot_statusrows (300 frozen = 300 approved, no drift). NVFLARE then reused the same project (--project-id): the image-pull wait cleared instantly off the frozen set and training ran on the identical parquet.GET /projects/{id}/cohort-snapshotsverified against the live hub.build=True— the integration stack used to reuse a stale image, so a dependency change (pyarrow here) silently never reached it and the suite green-lit code that could not run in a fresh build.security.rstclaimed cached results "expire in minutes" (never true — 60-day query-keyed cache); it now describes the frozen-cohort model.🤖 Generated with Claude Code