Backfill offeringId onto offering-contained documents [CLUE-643] - #2980
Open
scytacki wants to merge 20 commits into
Open
Backfill offeringId onto offering-contained documents [CLUE-643]#2980scytacki wants to merge 20 commits into
scytacki wants to merge 20 commits into
Conversation
…CLUE-643] Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scan is ordered by document id, so two documents belonging to the same user essentially never land in the same page and a per-user cache would hit approximately never. Getting the clustering that would make caching pay off means ordering by uid, which needs a new composite collection-group index. Also record that the extracted lookup must avoid import.meta so Jest can load it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
] A transport error is not evidence that a document has no offering, so it gets its own census bucket rather than joining the two "not found" ones. The extracted lookup throws and each caller decides what to do. Also pin the classification order: class-wide is decided before alreadySet, so a class-wide document that wrongly carries an offeringId is still reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-643] Adds the paginated collection-group scan that census-tallies every offering-contained document by outcome, type, and space, with RTDB lookups run concurrently per page. Nothing is written yet; dryRun is honored and written stays 0 until a later task drains the resolved lookups into batched writes. Also adds a Jest moduleNameMapper rule stripping a relative import's trailing `.js` extension so ts-jest's default resolver can find the sibling `.ts` file. scripts/ is an ESM package (`"type": "module"`), so its local imports use the `.js` extension per Node's ESM rules, but ts-jest never transpiles that extension away for value imports, and Jest's resolver treats an explicit extension as literal. This is the first script under test that imports another script module as a value rather than only a type, so the gap was previously unexercised.
] Jest resolves only the extensionless specifier to the sibling .ts file, so the tested script uses that form. tsx resolves either, verified against the module. Reverts the moduleNameMapper entry, which rewrote every relative .js import in the repo to fix a scripts-only resolution problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d report on crash [CLUE-643] Applies the final code review's fix wave to backfill-document-offering-id.ts: derive the Realtime Database URL from the service account instead of hardcoding it, so the script cannot read one environment's offerings and write them onto another's documents; count `written` only after a batch commits so a failed run cannot over-report what landed; print the census in a `finally` and log per-type progress so a crash still leaves a record of how far the run got; and reject a non-string offeringId in the shared RTDB lookup so a legacy numeric value can't silently fail the app's string-typed Firestore query. Adds tests for the new type check, the default dry-run behavior, and per-bucket accounting in the mixed-bucket test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects three claims that implementation made untrue: per-type queries are not independently resumable, group documents are exempt from the uid derivation rather than from repair, and the order-by uses the "__name__" string so the module needs no runtime firebase-admin import. Adds an "Operating the run" section for the properties that matter to whoever points this at production data: the database URL is derived from the credential rather than named, `written` counts committed writes so a dying run under-reports rather than over-reports, a failed run still prints its census, and there is no resume. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…E-643] The two Firebase projects do not share an RTDB host pattern: production is collaborative-learning-ec215.firebaseio.com, staging is collaborative-learning-staging-default-rtdb.firebaseio.com. Deriving the URL from the project id produced a wrong host for staging, so the script now looks it up and fails loudly on an unknown project rather than guessing. DATABASE_URL overrides. Also correct the index claim, which a staging run disproved: the collection group index on documents.type is declared in firestore.indexes.json, but that file was reconciled against production, so each environment needs it actually deployed. The first query fails outright without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nown [CLUE-643] A staging census scanned 209 documents, of which 68 were qa and 46 dev. Those roots are keyed by user id rather than portal, so they matched no space and were labelled "unknown" — while being counted under buckets that say nothing is wrong, because they all short-circuited as alreadySet or skippedClassWide first. A partition document that actually needed repair would have reported as an unrecognized path shape, which reads as an anomaly rather than scratch data. They now get their own bucket, asked before every other question because it is a matter of scope rather than a property of the document. unknownSpace is left meaning what it says. The space label now travels with the classification, so the path is parsed once instead of once per document at each of two sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LUE-643] TYPES limits the scan to a subset, for a cheap first look before committing to a full sweep; PAGE_SIZE tunes the query page. Both are parsed and validated before anything connects, and an unrecognized type name throws rather than scanning nothing — a typo would otherwise produce a clean empty census that reads exactly like "this type has no problems". The run header now echoes types, page size and mode, so what a run did is recoverable from its log. Also correct the index claim again: the collection group index on documents.type is deployed in neither staging nor production. It was added to the file alongside backfill-group-document-axes.ts, which needs the same index and has not been run. It is a prerequisite for the sweep in every environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-643] It was created in staging and production on 2026-08-13, and indexes persist, so CLUE-604 inherits a satisfied prerequisite rather than a step to repeat. Kept as a note because the symptom is otherwise baffling: without it the first query fails outright, identically for both sweep scripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-643] The qa and dev partitions are no longer skipped. Both stores derive their root from the same getRootId, so the Firestore root id is also the RTDB root id; the only missing piece was the portal segment, which sampling production settles as "localhost" for dev and "qa" for qa. `test` takes an arbitrary portal and still cannot be derived, so it reports as an unknown space rather than being guessed at — production has no test documents. A production census also found curriculum supports keyed like "2.2 Initial Challenge Support 1". A "." is illegal in an RTDB path, so those lookups throw and were counted as lookupError, which reads as transient when the failure is permanent. They now get their own bucket, decided before the lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
112,900 documents scanned. Across real data 71,399 need an offeringId and 53 cannot get one — a 99.93% recovery rate from the single source this script uses, which settles the question the spec left open: do not build the offering-tree or fuzzy portal sources. Also records what the per-root split shows and the totals hide: 88% of the unrecoverable residue is qa, whose RTDB side delete-qa-user-data.ts purges while leaving the Firestore metadata behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scanning all 146,105 documents in the collection group confirmed they were the only two with an RTDB-illegal path segment, and both were deleted on 2026-08-20, so keyNotRtdbSafe now reports zero and the real-data residue is 51 rather than 53. Worth recording that neither carried a `unit`, so isInClassUnitContainer could never have misread them — removing them was housekeeping rather than a fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The backfill was scoped under CLUE-610 on the theory that shipping the axes work without it would break something for users. It would not: the affected documents never carry `concurrent`, so they cannot reach the container guard that reads `offeringId`'s absence, and every group document -- the only population that can reach it -- already has one. It belongs with the other repairs of the Firestore metadata mirror, which run their own sweeps rather than waiting on CLUE-604's release gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ffering-id-backfill
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2980 +/- ##
===========================================
- Coverage 85.97% 70.58% -15.39%
===========================================
Files 989 986 -3
Lines 56618 56749 +131
Branches 14953 14994 +41
===========================================
- Hits 48677 40058 -8619
- Misses 7921 16655 +8734
- Partials 20 36 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
collaborative-learning
 |
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-643-document-offering-id-backfill
|
| Run status |
|
| Run duration | 03m 33s |
| Commit |
|
| Committer | Scott Cytacki |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
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
1 participant
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.
A repair script only — no product code changes, and nothing here has been run against a database in
apply mode.
The problem
isInClassUnitContainer(src/models/document/document-axes.ts) identifies a document's container bythe absence of
offeringId. So an offering-contained document that has noofferingIdon itsFirestore metadata reads as belonging to the class's copy of the unit — the wrong container axis.
71,399 documents across
authedanddemoare in that state. This script recovers the value fromthe realtime database, where the document's own metadata node has carried it all along, and writes
it back.
What's here
Documents are scanned by collection group, one
typeat a time, using thecontainerType: "offering"types from document-kinds.ts. Each candidate is classified into exactly one bucket, and only the
resolvedbucket is written —set({ offeringId }, { merge: true }), batched, with the written countincremented only after a commit resolves.
"publication"appears in the type list because that is the problem publication's stored value;ProblemPublicationin document-types.ts is the constant's name, not its value. Both"group"and"axes"appear because the generic axes type is mid-rename, and accepting both is what lets thissweep and
backfill-group-document-axes.tsrun in either order.What the production census found (2026-08-20)
A full dry run scanned 112,900 documents in 4m24s.
autheddemodevqa* Both were deleted on 2026-08-20, so a re-run reports zero. They were
supportPublicationrows from2021 keyed
2.2 Initial Challenge Support 1and2.2 Support 1— curriculum-authored supports whosekeys contain a
., which a realtime-database path forbids. Scanning all 146,105 documents in thecollection group confirmed they were the only two, and neither carried a
unit, so they neverendangered
isInClassUnitContainerin the first place.A 99.93% recovery rate across real data, with 51 documents unrecoverable. That settles a question
the design deliberately left open: two further recovery sources were sketched (walking the offering
tree, and a fuzzy match against the portal API), and neither is worth building to rescue fifty-odd
documents. "Report and leave alone" is the policy, and it is what the script already does.
nodeWithoutOfferingIdis 0 across all 112,900 documents. There is no population of realtimenodes written without an offering; the only failure mode is the node being absent entirely.
Safety
Dry run by default,
APPLY=1to write, mode printed at startup.TYPES=limits the scan to a subsetof types and
PAGE_SIZE=tunes the query page, so a large environment can be sampled before a fullsweep is committed to. The census prints from a
finallyblock, so a run that dies still says what itdid.
Idempotency works differently here than in a create-only script: a second run reclassifies the
documents the first one wrote as
alreadySetrather than rewriting them.Requires a deployed index
The collection-group queries need the single-field
COLLECTION_GROUPindex ontype, declared infirestore.indexes.json. It was deployed to staging and production on 2026-08-13, so neither
environment needs anything done. A new environment would need it created before this script runs at
all — the first query fails outright without it. Diff against the deployed indexes before deploying:
an environment may carry indexes absent from the file, which a
--forcedeploy would delete.Sequencing
No app code, no rules change, no client behavior change, so this needs no drain, no deploy window and
no place in the release chain.
It does need to run after the two repairs in #2977. This script finds a document's
realtime-database node through its
context_id, so a wrong one makes the document look unrecoverable;and rows that do not exist yet cannot be scanned at all.
That predicts something worth checking when the repairs run: the 43
authednoMetadataNodedocuments should shrink, because some of them are likely the documents whose
context_idnames thewrong class. If the count does not move, the two populations are disjoint and that is worth knowing
too.
Against CLUE-604's axes backfill it is order-independent, because this script accepts either side of
that rename.
Testing
58 unit tests, against a mock Firestore and a mock realtime database. Worth a look in review: the
bucket classification table, the pagination boundary, and
isRtdbAddressable— a key containing.,#,$,[,]or/cannot appear in a realtime-database path, and a lookup on one throwsrather than returning empty.
Not in this PR
Running it.
APPLY=1needs a separate go-ahead, after the repairs in #2977 have been applied.🤖 Generated with Claude Code