From c60295e2641bf8cc57a6fe9766f6652a7ecd17bd Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 21:35:26 +0200 Subject: [PATCH] fix: thumbnails may be kept, the embedding model may not be changed here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Caching.** `/uploads` answered `private, no-store` for everything, so a page of forty thumbnails re-fetched forty images every time it was drawn. A derivative may now be kept for a week by the browser that asked for it — `private`, never a shared cache, because a shared cache in front of access-controlled images is how one learner is served another's figure. It is safe to keep because it cannot change: `thumbs/256/` is made once from an immutable original. Originals still say no-store. **The embedding model is env-only.** Every vector in the database came from it, and vectors from different models are not comparable — change it and search returns noise until 3,000 questions, 334 articles and every card have been re-embedded. The settings page now shows it as text with Test and Regenerate beside it, and the API refuses a change rather than ignoring one, naming `LITELLM_EMBEDDING_MODEL` in the refusal. **The figure audit retries and gives up.** Its second run met a proxy outage and reported all 327 figures unreadable, having changed nothing but spent the time. Three tries each with backoff now, and it aborts after twelve consecutive failures: a run that says "everything is unreadable" has told you nothing. **`.env.example` is complete.** It listed 23 of the 53 settings; it now lists all of them, grouped, each with the default it falls back to and — where it matters — what happens if it is wrong. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/.env.example | 96 ++++++++++++++++++++----- backend/app/routers/admin.py | 14 +++- backend/app/routers/uploads.py | 26 +++++-- backend/app/tasks/quiz_tasks.py | 51 +++++++++---- backend/tests/test_related_privacy.py | 20 ++++++ frontend/src/components/ModelsAdmin.jsx | 55 ++++---------- 6 files changed, 187 insertions(+), 75 deletions(-) diff --git a/backend/.env.example b/backend/.env.example index 6151291..1fa1f72 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -1,27 +1,76 @@ -# Database -DATABASE_URL=sqlite:////app/data/quiz.db +# Every setting the backend reads, with the default it falls back to. Blank +# means "off" or "not configured" throughout — nothing here has a secret in it, +# and nothing here is required except the database, the secret key and a model. + +# ── Database and sessions ────────────────────────────────────────────── +DATABASE_URL=postgresql://quiz:quiz@postgres:5432/quiz SECRET_KEY=change-me-to-a-random-secret-key-in-production ALGORITHM=HS256 ACCESS_TOKEN_EXPIRE_MINUTES=1440 -# Optional bootstrap admin. Leave blank to use first-user-becomes-admin registration. +# Optional bootstrap admin. Leave blank to use first-user-becomes-admin. DEFAULT_ADMIN_EMAIL= DEFAULT_ADMIN_PASSWORD= -# Redis (use service name in Docker) +# Redis: Celery's broker, the rate limiters, job progress and site settings. REDIS_URL=redis://redis:6379/0 -# AI - LiteLLM (supports OpenAI, Anthropic, etc.) +# ── Models ───────────────────────────────────────────────────────────── +# Everything goes through one LiteLLM proxy. Per-job models (extraction, the +# tutor, TTS, and so on) are chosen by an administrator in Settings → AI +# models; this is the fallback when a job has no model of its own. LITELLM_MODEL=gpt-4o-mini LITELLM_API_KEY=your-api-key-here +LITELLM_API_BASE= -# Local Sherpa speech gateway for self-hosted TTS/STT +# The embedding model, and the one model setting that is NOT editable in the +# interface. Every stored vector was produced by it, and vectors from different +# models are not comparable — change this and search returns noise until every +# question, article and card has been re-embedded. That is a deployment, so it +# lives here. `EMBEDDING_DIMENSIONS` must match what the model returns. +LITELLM_EMBEDDING_MODEL= +EMBEDDING_DIMENSIONS=1024 + +# The cross-encoder that reorders search results. Unset, unreachable or +# malformed and the fused ranking is returned untouched — never fewer results. +LITELLM_RERANK_MODEL=cohere-rerank-v4.0-pro + +# Direct provider keys, used only where the proxy does not carry the service. +OPENAI_API_KEY= +ELEVENLABS_API_KEY= +GOOGLE_TTS_API_KEY= +AWS_ACCESS_KEY_ID= +AWS_SECRET_ACCESS_KEY= +AWS_REGION=us-east-1 +AWS_BEDROCK_REGION=us-east-1 + +# Self-hosted speech, for TTS and dictation without leaving the machine. LOCAL_SPEECH_GATEWAY_URL=http://local-speech-gateway:8110 -# Vector store +# ── Where the site lives ─────────────────────────────────────────────── +# Used in the links inside emails, so a wrong value sends people nowhere. +APP_URL=https://pedshub.com +LOG_LEVEL=INFO + +# ── Storage ──────────────────────────────────────────────────────────── +# `local` keeps uploads on the volume at UPLOAD_DIR; `s3` puts them in a +# bucket and reads them back through it. Thumbnails live beside the original +# either way. +STORAGE_BACKEND=local +UPLOAD_DIR=/app/uploads +MAX_UPLOAD_SIZE=524288000 +S3_ENDPOINT_URL=http://minio:9000 +S3_ACCESS_KEY= +S3_SECRET_KEY= +S3_BUCKET=pedshub-media +S3_REGION=us-east-1 + +# Page chunks for extraction context. CHROMA_PERSIST_DIR=/app/chroma_data -# SMTP Email for reminders +# ── Email ────────────────────────────────────────────────────────────── +# With MAIL_USERNAME or MAIL_FROM blank, mail is logged instead of sent — which +# also means sign-in codes and verification links go nowhere. MAIL_USERNAME=your-email@example.com MAIL_PASSWORD=your-app-password MAIL_FROM=your-email@example.com @@ -29,15 +78,30 @@ MAIL_PORT=587 MAIL_SERVER=smtp.gmail.com MAIL_STARTTLS=true MAIL_SSL_TLS=false +# Where the contact form's messages land. +ADMIN_EMAIL= -# File uploads -UPLOAD_DIR=/app/uploads -MAX_UPLOAD_SIZE=524288000 - -# Bot protection — Cap, self-hosted beside the app. Leave the secret blank to -# disable the challenge; -# the sign-up and contact forms then accept submissions without one. +# ── Bot protection ───────────────────────────────────────────────────── +# Cap, self-hosted beside the app. Leave the secret blank to disable the +# challenge; sign-up then accepts submissions without one. CAP_SECRET_KEY= CAP_SITE_KEY= -# Cap runs beside the app; this is its address on the compose network. CAP_API_URL=http://cap:3000 + +# ── Single sign-on ───────────────────────────────────────────────────── +# Any OIDC provider. Blank means the site uses its own accounts only. +OIDC_PROVIDER_URL= +OIDC_CLIENT_ID= +OIDC_CLIENT_SECRET= +OIDC_SCOPES=openid email profile +OIDC_PROVIDER_NAME=SSO + +# ── Live sessions ────────────────────────────────────────────────────── +BBB_SERVER_URL= +BBB_SECRET= + +# ── Clinical corpus (read-only) ──────────────────────────────────────── +# A separate Milvus collection the assistant may query but never write to. +CLINICAL_MILVUS_URI= +CLINICAL_MILVUS_TOKEN= +CLINICAL_MILVUS_COLLECTION=mcp_bge_m3_1024 diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 2a45420..861e483 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -606,6 +606,8 @@ def get_settings(admin: User = Depends(require_admin)): import redis as redis_lib r = redis_lib.from_url(settings.REDIS_URL, decode_responses=True) registration_enabled = r.get("settings:registration_enabled") + # Read for as long as an older deployment may still have one in Redis; + # nothing writes it any more. embedding_model = r.get("settings:embedding_model") rerank_model = r.get("settings:rerank_model") sso_only = r.get("settings:sso_only") @@ -650,8 +652,18 @@ def update_settings( if flag in settings_data: site_settings.set_flag(flag, bool(settings_data[flag])) + # Not settable here, and refused rather than ignored. Every vector in + # the database was produced by this model; changing it makes all of them + # incomparable and search returns noise until 3,000 questions, every + # article and every card have been re-embedded. A change that expensive + # belongs in the environment, where making it is a deployment somebody + # decided on — not a text field on a settings page. if "embedding_model" in settings_data: - r.set("settings:embedding_model", settings_data["embedding_model"]) + raise HTTPException( + 400, + "The embedding model is set by LITELLM_EMBEDDING_MODEL in the " + "environment. Changing it invalidates every stored vector, so it " + "is a deployment rather than a setting.") if "rerank_model" in settings_data: r.set("settings:rerank_model", (settings_data["rerank_model"] or "").strip()) diff --git a/backend/app/routers/uploads.py b/backend/app/routers/uploads.py index 96a5374..04ea814 100644 --- a/backend/app/routers/uploads.py +++ b/backend/app/routers/uploads.py @@ -27,9 +27,18 @@ def read_upload(path: str, request: Request, attempt_id: int | None = None, below is unchanged and runs first: a thumbnail of a file you may not read is a file you may not read. - These stay `private, no-store` like everything else here. They are behind - authentication, so there is nothing for a shared cache to do with them; the - win is that the bytes are a tenth the size. + A derivative may be kept by the browser that fetched it; an original may + not. `private` in both cases — never a shared cache, because a shared cache + in front of access-controlled images is how one learner is served another's + private figure. What that leaves is the requester's own browser, which has + already been allowed to see the bytes, and which was re-fetching every + thumbnail on every page for no reason. + + A derivative is safe to keep because it cannot change: `thumbs/256/` + is made once from an immutable original and never rewritten. Losing access + to an image does not evict it from that one browser's cache for a week, + which is the honest cost, and a small one for a picture that browser was + entitled to draw yesterday. """ headers = {"Cache-Control": "private, no-store", "Vary": "Cookie, Authorization", "X-Content-Type-Options": "nosniff"} if w is not None and w not in thumbnails.WIDTHS: @@ -42,6 +51,12 @@ def read_upload(path: str, request: Request, attempt_id: int | None = None, raise +#: A week in the requester's own browser. `immutable` so a reload does not +#: revalidate it: the derivative is made once from an original that never +#: changes, so there is nothing for a conditional request to discover. +DERIVATIVE_CACHE = "private, max-age=604800, immutable" + + def _read_upload(path, request, attempt_id, db, headers, width=None): try: path = local_upload_path(path) @@ -76,10 +91,11 @@ def _read_upload(path, request, attempt_id, db, headers, width=None): if width or thumbnails.needs_converting(path): small = thumbnails.get(path, width) if small is not None: + cached = {**headers, "Cache-Control": DERIVATIVE_CACHE} if request.method == "HEAD": return Response(status_code=200, media_type="image/webp", - headers={**headers, "Content-Length": str(len(small))}) - return Response(content=small, media_type="image/webp", headers=headers) + headers={**cached, "Content-Length": str(len(small))}) + return Response(content=small, media_type="image/webp", headers=cached) # Serving must go through the storage service, or object storage would be # write-only: the bytes would be in the bucket and still read from disk. diff --git a/backend/app/tasks/quiz_tasks.py b/backend/app/tasks/quiz_tasks.py index 4ea6888..9023190 100644 --- a/backend/app/tasks/quiz_tasks.py +++ b/backend/app/tasks/quiz_tasks.py @@ -391,6 +391,13 @@ DIFFICULTY_BATCH = 25 #: quietly become the thing that decides what "the tool model" means. FIGURE_AUDIT_MODEL = "openrouter-gemini-2.5-flash" +#: Two tries per figure, a second apart. A 502 from the proxy is usually a +#: moment rather than a state. +RETRIES = 3 +#: And if it *is* a state, stop. A run that reports every figure unreadable has +#: told you nothing and cost an hour. +GIVE_UP_AFTER = 12 + FIGURE_AUDIT_PROMPT = """This figure is attached to the exam question below. Decide whether it belongs to it. @@ -456,6 +463,8 @@ def audit_question_figures(self, job_id: str = "", limit: int | None = None, db = SessionLocal() model = model_id or FIGURE_AUDIT_MODEL seen = kept = detached = unsure = unreadable = described = 0 + consecutive_failures = 0 + last = None mismatches: list[dict] = [] try: rows = db.query(Question).filter( @@ -484,20 +493,38 @@ def audit_question_figures(self, job_id: str = "", limit: int | None = None, continue stem = (question.question_text or "")[:900] - try: - raw = (chat(model=model, max_tokens=300, temperature=0, messages=[{ - "role": "user", "content": [ - vision_service.image_part(image), - {"type": "text", "text": FIGURE_AUDIT_PROMPT + stem}, - ]}]) or "").strip() - if raw.startswith("```"): - raw = raw.split("\n", 1)[1] if "\n" in raw else raw[3:] - raw = raw[:-3] if raw.endswith("```") else raw - verdict = _json.loads(raw.strip()) - except Exception as exc: + verdict = None + for attempt in range(RETRIES): + try: + raw = (chat(model=model, max_tokens=300, temperature=0, messages=[{ + "role": "user", "content": [ + vision_service.image_part(image), + {"type": "text", "text": FIGURE_AUDIT_PROMPT + stem}, + ]}]) or "").strip() + if raw.startswith("```"): + raw = raw.split("\n", 1)[1] if "\n" in raw else raw[3:] + raw = raw[:-3] if raw.endswith("```") else raw + verdict = _json.loads(raw.strip()) + break + except Exception as exc: + last = exc + # A 502 from the proxy is usually a moment, not a state. + if attempt + 1 < RETRIES: + time.sleep(2 ** attempt) + if verdict is None: unreadable += 1 - logger.warning("Figure audit failed for question %s: %s", question.id, exc) + consecutive_failures += 1 + logger.warning("Figure audit failed for question %s: %s", question.id, last) + # A whole corpus of "unreadable" is not a result, it is an + # outage — and the first run of this marched through 327 + # questions reporting nothing while the proxy was down. Stop and + # say so, so the run can be repeated when it is back. + if consecutive_failures >= GIVE_UP_AFTER: + raise RuntimeError( + f"{consecutive_failures} figures in a row could not be read — " + f"the model or the proxy is unavailable. Last error: {last}") continue + consecutive_failures = 0 belongs = str(verdict.get("belongs", "unsure")).strip().lower() kind = str(verdict.get("kind", "")).strip().lower() diff --git a/backend/tests/test_related_privacy.py b/backend/tests/test_related_privacy.py index a256f6b..3ba5bf2 100644 --- a/backend/tests/test_related_privacy.py +++ b/backend/tests/test_related_privacy.py @@ -162,6 +162,26 @@ class PrivacyTests(unittest.TestCase): self.assertEqual(res.status_code, 200, res.text) self.assertEqual(res.headers['cache-control'], 'private, no-store') self.assertEqual(res.headers['vary'], 'Cookie, Authorization') + # A derivative may be kept by the browser that asked for it — never by + # a shared cache, which in front of access-controlled images is how one + # learner is served another's private figure. The original still says + # no-store; only the thumbnail, which cannot change, may be kept. + # A real image, because a derivative only exists where one can be made: + # the other fixtures here are the bytes `synthetic-image`, and asking + # for a thumbnail of those correctly gets the original back. + import io + from PIL import Image as PILImage + buffer = io.BytesIO() + PILImage.new('RGB', (900, 600), (200, 30, 30)).save(buffer, format='PNG') + self.file('questions/stem-1.png', buffer.getvalue()) + + thumb = self.client.get('/uploads/questions/stem-1.png?w=256') + self.assertEqual(thumb.status_code, 200, thumb.text) + self.assertIn('private', thumb.headers['cache-control']) + self.assertIn('max-age=', thumb.headers['cache-control']) + self.assertNotIn('public', thumb.headers['cache-control']) + self.assertEqual(thumb.headers['vary'], 'Cookie, Authorization') + # A course's images stay out of reach whoever asks. self.assertEqual(self.client.get('/uploads/questions/stem-5.png').status_code, 404) self.db.get(Question, 1).deleted_at = datetime(2026, 1, 1) diff --git a/frontend/src/components/ModelsAdmin.jsx b/frontend/src/components/ModelsAdmin.jsx index c1bfd6f..e594ae9 100644 --- a/frontend/src/components/ModelsAdmin.jsx +++ b/frontend/src/components/ModelsAdmin.jsx @@ -59,9 +59,7 @@ export default function ModelsAdmin() { const [allowFor, setAllowFor] = useState('teach') // Embeddings: one model for the whole site. - const [embedding, setEmbedding] = useState('') const [embeddingSaved, setEmbeddingSaved] = useState('') - const [embedOffered, setEmbedOffered] = useState([]) const [embedBusy, setEmbedBusy] = useState('') const [embedResult, setEmbedResult] = useState(null) @@ -69,9 +67,7 @@ export default function ModelsAdmin() { Promise.all([api.get('/admin/models'), api.get('/admin/settings')]) .then(([list, settings]) => { setModels(list.data || []) - const current = settings.data?.embedding_model || '' - setEmbedding(current) - setEmbeddingSaved(current) + setEmbeddingSaved(settings.data?.embedding_model || '') }) .catch(() => setError('Could not load the models')) .finally(() => setLoading(false)) @@ -142,14 +138,6 @@ export default function ModelsAdmin() { 'Could not remove that model') } - const saveEmbedding = (modelId) => act('embedding', - async () => { - await api.put('/admin/settings', { embedding_model: modelId }) - setEmbedding(modelId) - setEmbeddingSaved(modelId) - }, - 'Could not save that model', - 'Saved. Vectors made by the previous model are not comparable — regenerate them.') const testEmbedding = async () => { setEmbedBusy('test'); setEmbedResult(null) @@ -173,14 +161,6 @@ export default function ModelsAdmin() { finally { setEmbedBusy('') } } - const offerEmbeddings = async () => { - setEmbedBusy('find'); setError('') - try { - const res = await api.post('/admin/litellm/models', { mode: 'embedding' }) - setEmbedOffered(res.data.models || []) - } catch (err) { setError(detail(err, 'Could not reach the proxy')) } - finally { setEmbedBusy('') } - } const byJob = useMemo(() => { const grouped = Object.fromEntries(JOBS.map(job => [job.key, []])) @@ -254,23 +234,14 @@ export default function ModelsAdmin() { {EMBEDDING.hint}
- {embedOffered.length > 0 ? ( - - ) : ( - setEmbedding(e.target.value)} /> - )} - - + {/* Read-only, and it is the one setting on this page that is. Every + vector in the database was produced by this model; changing it + makes all of them incomparable, and search quietly returns + nonsense until 3,000 questions, 334 articles and every card have + been re-embedded. That is not a thing to have behind a text field + on a settings page, so it lives in the environment where changing + it is a deployment. */} + {embeddingSaved || '(not set)'}