Bug/sc 45009/title rename leaves index key versionstate - #3442
Merged
stevekaplan123 merged 33 commits intoAug 4, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Sefaria’s startup/library-build path by adding per-record exception guards so that a single malformed MongoDB record (Index/Topic/Term/Category/Lexicon/User profile, etc.) is logged and skipped instead of preventing the server from booting.
Changes:
- Introduces a shared
BAD_RECORD_EXCEPTIONStuple and uses it to wrap startup-time per-record loops. - Makes Index map building, topic TOC/category mapping, and term/topic mappings resilient to malformed records.
- Adds similar skip-and-log behavior to linker trie initialization, TOC tree building, autocomplete user ingestion, and lexicon trie ingestion.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sefaria/system/exceptions.py | Adds BAD_RECORD_EXCEPTIONS intended for “single bad record” startup guards. |
| sefaria/model/text.py | Adds per-record guards around index-map build, topic TOC recursion/sorting, term mappings, topic mappings, and index enumeration. |
| sefaria/model/linker/match_template.py | Wraps match-template trie building per node to prevent one bad node from aborting trie creation. |
| sefaria/model/linker/category_resolver.py | Makes category matcher initialization resilient to malformed templates/terms. |
| sefaria/model/category.py | Adds per-record guards when building the TOC tree (vstate docs, first-comment links, index placement, collection placement). |
| sefaria/model/autospell.py | Adds per-record guards for user-profile autocomplete ingestion and lexicon entry trie ingestion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+276
to
+281
| try: | ||
| self._collections_in_library.append(c.slug) | ||
| node = TocCollectionNode(collection_object=c) | ||
| categories = node.categories | ||
| cat = self.lookup(node.categories) | ||
| if not cat: |
stevekaplan123
commented
Jun 28, 2026
stevekaplan123
commented
Jun 28, 2026
Comment on lines
70
to
+73
| for node in nodes: | ||
| for match_template in node.get_match_templates(): | ||
| if not node.is_root() and not match_template.matches_scope(self.scope): | ||
| continue | ||
| curr_dict_queue = [trie] | ||
| self.__add_all_term_titles_to_trie(match_template.terms, node, curr_dict_queue) | ||
| self.__add_nodes_to_leaves(node, curr_dict_queue) | ||
| # One node with a corrupt match_template/term/schema must not abort startup. | ||
| with skip_bad_record("init_library_cache", "MatchTemplateTrie node", record=str(node)): | ||
| for match_template in node.get_match_templates(): |
Comment on lines
+18
to
+23
| from contextlib import contextmanager | ||
| from collections import defaultdict, namedtuple | ||
| from sefaria.system.exceptions import BAD_RECORD_EXCEPTIONS | ||
| from sefaria.helper.slack.send_message import notify_engineering_signal | ||
|
|
||
|
|
Comment on lines
+69
to
+75
| global _skip_saw_error | ||
| key = (pathway, what) | ||
| _skip_group_counts[key] += 1 | ||
| if _skip_group_counts[key] <= MAX_STORED_PER_GROUP: | ||
| skip_records.append(SkipRecord(pathway, what, record, level, error_type, detail)) | ||
| if level == "error": | ||
| _skip_saw_error = True |
Comment on lines
+106
to
+126
| if _skip_group_counts: | ||
| total = sum(_skip_group_counts.values()) | ||
| # Group the stored records by (pathway, what), preserving their (bounded) detail. | ||
| grouped = defaultdict(list) | ||
| for rec in skip_records: | ||
| grouped[(rec.pathway, rec.what)].append(rec) | ||
|
|
||
| lines = [] | ||
| for (pw, what), count in _skip_group_counts.items(): | ||
| lines.append(" [{}] {}: {}".format(pw, what, count)) | ||
| stored = grouped.get((pw, what), []) | ||
| for rec in stored: | ||
| lines.append(" - {}".format(_format_skip_record(rec))) | ||
| if count > len(stored): | ||
| lines.append(" … {} more".format(count - len(stored))) | ||
|
|
||
| message = "[pathway:{}] cache build skipped {} bad record(s):\n{}".format( | ||
| pathway, total, "\n".join(lines)) | ||
| notify_engineering_signal(message, level="error" if _skip_saw_error else "warning") | ||
| reset_skip_counts() | ||
|
|
Comment on lines
+140
to
+145
| def reset_skip_counts(): | ||
| """Clear the skip log, group counts, and error flag. Called after each build's summary is posted.""" | ||
| global _skip_saw_error | ||
| skip_records.clear() | ||
| _skip_group_counts.clear() | ||
| _skip_saw_error = False |
stevekaplan123
marked this pull request as ready for review
July 1, 2026 12:29
…45009/title-rename-leaves-index-key-versionstate
yodem
reviewed
Jul 20, 2026
Member
Author
|
Skip log is mutable global state without a lock |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
sefaria/model/linker/match_template.py:72
record=str(node)is evaluated before entering theskip_bad_recordcontext. Ifnode.__str__/__repr__touches corrupt schema fields (the exact scenario this guard is meant to survive), it can raise outside the guard and still abort startup. Use a cheap/safe identifier forrecordinstead ofstr(node).
# One node with a corrupt match_template/term/schema must not abort startup.
with skip_bad_record("startup", "MatchTemplateTrie node", record=str(node)):
Comment on lines
+130
to
+132
| fullname = f"{u.first_name or ''} {u.last_name or ''}" | ||
| normal_name = self.normalizer(fullname) | ||
| self.title_trie[normal_name] = { |
Comment on lines
+145
to
+150
| # Detail (B): a bold group header, bulleted records, with the site's own pathway | ||
| # noted only when it differs from the build pathway in the header. | ||
| lines = [header] | ||
| for (pw, operation), count in groups: | ||
| lines.append("\n*{}* — {}".format(operation, count)) | ||
| stored = grouped.get((pw, operation), []) |
Member
Author
|
@yitzhakc Test 1 — test_concurrent_skips_and_summaries_lose_nothing. 8 threads record 200 skips each while the main thread repeatedly posts summaries; asserts all 1,600 show up across those summaries. (Note the sys.setswitchinterval(1e-6) fixture: Python switches threads only every 5ms by default, which is longer than the whole risky window, so without it the threads never interleave and the test passes even with the lock deleted). The fixture restores the original value in a finally — it's process-wide and would slow the rest of the suite. Test 2 — test_lock_is_not_held_while_posting_to_slack. Guards the fact that the lock is released before the Slack call. Wrapping the whole function in one with _lock: would deadlock startup on any re-entrant path (threading.Lock isn't reentrant) and would reset away skips recorded during delivery. The test's fake Slack call checks _lock.acquire(blocking=False) succeeds and records a skip, then asserts that skip survives. Non-blocking on purpose: a blocking acquire would hang the suite instead of failing. |
The Weblate sync in #3566 added "search.exactMatchToggle.allResults" and "search.exactMatchToggle.exactMatch" to i18n/interface/{en,he}.json. Those camelCase keys fail the ID shape enforced by Sefaria._keyedStringIdRegex, breaking the Jest suite on master. Widen the regex to allow uppercase letters anywhere except the first character of each dot-separated segment. That keeps the guard that matters: _isKeyedStringId() is a classifier, not just a validator -- Sefaria._() uses it to tell translation keys from plain data values, and Sefaria's data values are capitalized ("Gen.1", "b.Berakhot"). Requiring a lowercase (or digit/underscore) first character preserves that distinction. Verified against all 592 distinct keys across the four JSON maps: none are rejected, including sheets.1_person_likes_this_sheet, which starts a segment with a digit. No plain-English string that was safe before is misclassified now. Added a regression test covering both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
strings.test.js duplicated the _keyedStringIdRegex literal, so a change to the router in sefaria.js would leave the test asserting against a stale pattern -- passing while real keys had become unroutable. Import it from Sefaria instead. The test's stated contract is "every key matches the router regex", which is only literally true when it uses the router's own regex. Behavior stays pinned by the example-based _isKeyedStringId test, which hardcodes concrete accept/reject cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ename-leaves-index-key-versionstate
…e-leaves-index-key-versionstate
stevekaplan123
enabled auto-merge
August 4, 2026 15:34
yitzhakc
approved these changes
Aug 4, 2026
📊 Code Quality Score: 58/100
Was this score accurate? 👍 Yes · 👎 No Scored by GitVelocity · How are scores calculated? |
stevekaplan123
deleted the
bug/sc-45009/title-rename-leaves-index-key-versionstate
branch
August 4, 2026 16:20
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.
Description
The server failed to boot with KeyError: 'Rashi on Genesis 2' in all_index_records(). Root cause: _index_map is keyed by i.title while _index_title_maps is keyed by nodes.key; these diverge when an Index's title ≠ its root node key, and the lookup raised. This is one instance of a pervasive anti-pattern on the startup path: per-record loops over DB records with no per-item error handling, so any single malformed record (Index, Topic, Term, Collection, Lexicon entry, user profile, vstate doc, link) aborts the whole build and prevents server boot.
Code Changes
Per-record try → log → skip guards across the boot path so one bad record is logged and skipped. In
skip_tracking.pywe store the logs and report them all together in one Slack message.Example of logs:
Example of Slack message:
(In the most up-to-date code, instead of showing the confusing "init_library_cache", it shows "startup".