[Hardening] Replace builtin hash() with a deterministic digest in lore match chunk indexing #88

New issue

Closed

opened 2026-07-14 19:47:07 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-07-14 19:47:07 +00:00

Contributor

Context

The lore-extraction pipeline checkpoints per-type match results in lore_extract_cache using synthetic negative chunk_index values derived from Python's builtin hash():

webapp/backend/app/tasks/reminder_tasks.py:661 — stores match rows at type_index = -(abs(hash(entry_type)) % 10_000 + 1) (comment at 657-658 explains the negative-index scheme distinguishes them from real chunk rows, which are >= 0)
webapp/backend/app/tasks/reminder_tasks.py:723-725 — the consolidation step recomputes expected_indices = {-(abs(hash(t)) % 10_000 + 1) for t in expected_types} and waits until all are present

Python's hash() on strings is salted by PYTHONHASHSEED, which is randomized per interpreter process. PYTHONHASHSEED is not pinned anywhere in this repo (verified). Celery prefork children inherit the parent's seed, so the happy path works — every process in one worker instance computes the same indices.

Current behavior

If the Celery worker restarts mid-pipeline (deploy, OOM, crash), the new interpreter has a new hash seed. Match rows already written under the old seed become unfindable: lore_consolidate_proposals recomputes different expected_indices, never sees the checkpointed rows, retries to MaxRetriesExceeded, and fails the run ("Consolidation timed out"). This defeats the exact crash-recovery purpose of the checkpoint cache. The same mismatch occurs with more than one worker container, since each process has its own seed.

Fix / Spec

Replace both hash(...) expressions with a deterministic digest, e.g.:
```
import hashlib
def _type_index(entry_type: str) -> int:
    return -(int(hashlib.sha1(entry_type.encode()).hexdigest(), 16) % 10_000 + 1)
```
(Or use the ordinal of the sorted expected_types list — either is fine; keep the negative-index scheme so match rows stay distinguishable from chunk rows.)
Use the single helper at both sites (:661 and :723-725) so they cannot drift.
No migration needed: lore_extract_cache is a transient checkpoint cache. In-flight rows written under the old hash() scheme will simply miss once and the pipeline re-runs that step.

Acceptance criteria

Unit test asserting the index function is stable across interpreter invocations (e.g. assert exact precomputed constants for a couple of known entry types).
Consolidation finds match rows written by a different process (test can simulate by writing a row with the helper and reading with a freshly computed set).
Both call sites share one helper; no remaining hash( usage for cache indexing in reminder_tasks.py.

References

webapp/backend/app/tasks/reminder_tasks.py:657-672 (write side), :716-726 (read side / expected_indices)
Repo-wide: no PYTHONHASHSEED pin exists

Filed from the July 2026 full-project review.

## Context The lore-extraction pipeline checkpoints per-type match results in `lore_extract_cache` using synthetic negative `chunk_index` values derived from Python's builtin `hash()`: - `webapp/backend/app/tasks/reminder_tasks.py:661` — stores match rows at `type_index = -(abs(hash(entry_type)) % 10_000 + 1)` (comment at 657-658 explains the negative-index scheme distinguishes them from real chunk rows, which are >= 0) - `webapp/backend/app/tasks/reminder_tasks.py:723-725` — the consolidation step recomputes `expected_indices = {-(abs(hash(t)) % 10_000 + 1) for t in expected_types}` and waits until all are present Python's `hash()` on strings is salted by `PYTHONHASHSEED`, which is randomized per interpreter process. `PYTHONHASHSEED` is **not pinned anywhere in this repo** (verified). Celery prefork children inherit the parent's seed, so the happy path works — every process in one worker instance computes the same indices. ## Current behavior If the Celery worker restarts mid-pipeline (deploy, OOM, crash), the new interpreter has a new hash seed. Match rows already written under the old seed become unfindable: `lore_consolidate_proposals` recomputes different `expected_indices`, never sees the checkpointed rows, retries to `MaxRetriesExceeded`, and fails the run ("Consolidation timed out"). This defeats the exact crash-recovery purpose of the checkpoint cache. The same mismatch occurs with more than one worker container, since each process has its own seed. ## Fix / Spec 1. Replace both `hash(...)` expressions with a deterministic digest, e.g.: ```python import hashlib def _type_index(entry_type: str) -> int: return -(int(hashlib.sha1(entry_type.encode()).hexdigest(), 16) % 10_000 + 1) ``` (Or use the ordinal of the sorted `expected_types` list — either is fine; keep the negative-index scheme so match rows stay distinguishable from chunk rows.) 2. Use the single helper at both sites (`:661` and `:723-725`) so they cannot drift. 3. No migration needed: `lore_extract_cache` is a transient checkpoint cache. In-flight rows written under the old `hash()` scheme will simply miss once and the pipeline re-runs that step. ## Acceptance criteria - Unit test asserting the index function is stable across interpreter invocations (e.g. assert exact precomputed constants for a couple of known entry types). - Consolidation finds match rows written by a different process (test can simulate by writing a row with the helper and reading with a freshly computed set). - Both call sites share one helper; no remaining `hash(` usage for cache indexing in `reminder_tasks.py`. ## References - `webapp/backend/app/tasks/reminder_tasks.py:657-672` (write side), `:716-726` (read side / `expected_indices`) - Repo-wide: no `PYTHONHASHSEED` pin exists _Filed from the July 2026 full-project review._

claude-bot added the

backend

bug

labels

2026-07-14 19:47:14 +00:00

claude-bot added this to the v3.3.0 - Hardening: Bugs & Security milestone

2026-07-14 19:47:15 +00:00

claude-bot commented

2026-07-14 20:26:35 +00:00

Author

Contributor

Picking this up as part of a v3.3.0 push. Landing on branch hardening/backend together with #90, #97, #106, and #109 (grouped by component to keep the diffs reviewable).

Picking this up as part of a v3.3.0 push. Landing on branch `hardening/backend` together with #90, #97, #106, and #109 (grouped by component to keep the diffs reviewable).

claude-bot referenced this issue

2026-07-14 20:26:41 +00:00

[Hardening] Remove duplicate reminder delivery on session reschedule #90

claude-bot referenced this issue

2026-07-14 20:26:42 +00:00

[Hardening] Validate campaign discord_webhook_url against a Discord host allowlist (SSRF) #97

claude-bot referenced this issue

2026-07-14 20:26:45 +00:00

[Hardening] Reject placeholder and weak secrets at startup #106

claude-bot referenced this issue

2026-07-14 20:26:47 +00:00

[Hardening] Escape HTML in recap emails; stop echoing raw LLM errors from /ask #109

claude-bot commented

2026-07-14 21:33:34 +00:00

Author

Contributor

Fixed on main (commit 7977af7, merged via 1c9c19f). Added a single lore_match_chunk_index() helper (sha1-based, keeps the negative-index scheme) used at both the write site (lore_match_category) and read site (lore_consolidate_proposals) so they can't drift. Tests assert exact precomputed constants (npc=-2264, location=-8788, concept=-5198, item=-6992), stability across two subprocesses with differing PYTHONHASHSEED (and confirm builtin hash() genuinely differs across those seeds, so the test isn't vacuous), that consolidation finds rows written under the helper's index, and that no hash( remains for cache indexing. Backend suite green (358 passed), ruff clean.

Fixed on `main` (commit `7977af7`, merged via `1c9c19f`). Added a single `lore_match_chunk_index()` helper (sha1-based, keeps the negative-index scheme) used at both the write site (`lore_match_category`) and read site (`lore_consolidate_proposals`) so they can't drift. Tests assert exact precomputed constants (`npc=-2264`, `location=-8788`, `concept=-5198`, `item=-6992`), stability across two subprocesses with differing `PYTHONHASHSEED` (and confirm builtin `hash()` genuinely differs across those seeds, so the test isn't vacuous), that consolidation finds rows written under the helper's index, and that no `hash(` remains for cache indexing. Backend suite green (358 passed), ruff clean.

claude-bot closed this issue

2026-07-14 21:34:05 +00:00