Fix runtime SQLite contention and inference reliability

[protostatis]

Summary

• harden shared SQLite access with bounded busy waits, rollback-safe transactions, staggered writer schedules, and non-blocking event-loop boundaries
• make configured OpenRouter embeddings fail closed and score each post title/segment set in one batch without changing asymmetric scoring
• modernize the experimental inference command to read live sentiment/confounder tables and suppress directional output when social sentiment is stale

Motivation

Production validation found transient database is locked failures when scheduled writers aligned, a silent local fallback path for explicitly configured OpenRouter embeddings, up to 21 serial embedding calls per post, and a legacy inference path reading stale sentiment_scores rows.

Implementation

SQLite/runtime

• add shared 30-second SQLite busy-timeout helpers for sync and async connections
• serialize long-lived async writes and always roll back failed transactions
• close or roll back short synchronous write transactions safely
• stagger recurring writer phases and test eight hours of recurrence for exact collisions
• move blocking score, confounder, weight-loading, and SQLite work off async scheduler/API event loops
• run synchronous dashboard handlers in FastAPI thread workers

Embeddings

• remove silent OpenRouter-to-local fallback and reject unknown configured backends
• factor embedding scoring so batch and individual centroid/top-k/asymmetric results are equivalent
• batch the title plus up to 20 post segments into one provider call

Experimental inference

• replace legacy sentiment_scores reads with user_sentiment_scores and recent confounders
• include only a recent Fear & Greed observation
• return neutral/zero-confidence output when no fresh social source exists
• label the command as diagnostic rather than the production signal path

Validation

• pytest tests/ -q: 249 passed, 1 skipped
• Ruff import checks for all touched Python files: passed
• full Ruff checks for new helper/test files: passed
• Python compileall: passed
• dashboard frontend production build: passed
• game lint, 48 tests, and production build: passed
• game server 60 tests and syntax check: passed
• API Docker image build and import smoke test: passed

Operational notes

• no schema migration and no SQLite journal-mode change
• local embeddings remain the default; selecting EMBEDDING_BACKEND=openrouter now requires a valid OpenRouter configuration and never changes embedding spaces silently
• unrelated Reddit recovery artifacts remain isolated from this branch

[protostatis]

Sky's Code Review

This PR hardens SQLite access (shared 30s busy-timeout, rollback-safe sync/async transactions, staggered scheduler writers, offloading blocking work from event loops), makes OpenRouter embeddings fail-closed with no silent local fallback, batches per-post embedding calls (title + up to 20 segments), and modernizes the experimental inference command to read live user_sentiment_scores/confounders with neutral suppression when social sentiment is stale. The changes are well-motivated, well-tested (249 passing), and generally correct. The write_transaction/sqlite_transaction helpers are clean and correctly guarantee rollback. I have only minor-to-moderate concerns, chiefly a behavior change in migration error handling that flips silent-ignore to fail-hard, and a couple of code-clarity issues around early returns inside transaction context managers.

Verdict: Comment

Comments

• insert_sentiment_score in storage/db.py appears to still use the old raw self.conn.execute(...) + await self.conn.commit() path rather than write_transaction() (the diff shows its signature line but no body change). This leaves one write path un-serialized and non-rollback-safe relative to the others, which is inconsistent with the PR's stated goal. Worth confirming it was intentionally left out of scope or wrapping it too.
• save_post_score now calls get_or_create_user as a SEPARATE committed transaction before opening the insert transaction. This introduces a window where a user_profiles row is created/updated but the subsequent score insert can still fail, leaving an orphan last_seen bump (not a correctness bug since last_seen updates are idempotent, just noting the atomicity boundary changed from the original single-connection design).
• The staggered scheduler uses a single anchor = datetime.now(timezone.utc) and offset-based start dates. Tests verify 8 hours of non-collision, but note that intervals that are exact multiples of each other (e.g. crawl at 60s and price at 300s) can still drift back into alignment if job execution durations exceed the stagger offsets under load — the bounded busy-timeout plus max_instances=1 provides the real backstop, so this is acceptable defense-in-depth.
• Overall the fail-closed embedding change is the most impactful and is correctly implemented with regression tests asserting LocalSentenceTransformerProvider is never called on the OpenRouter error path. No secrets, no schema migration, and no journal-mode change are involved, which reduces rollout risk. Consider a follow-up to emit a clear startup log line when EMBEDDING_BACKEND=openrouter is selected with an empty key so operators see the reason for the hard failure.

---

Reviewed by Sky — Unchained Sky engineering agent

[protostatis]

Behavior change: the previous code silently swallowed ALL migration errors (comment 'Column already exists'). Now only 'duplicate column name' is tolerated and every other exception is re-raised after rollback. This is defensible hardening, but it means any previously-benign migration failure (e.g. an idempotency guard that returns a different message, or a rename/check that already applied) will now crash connect() and take the whole service down on startup. Verify that every existing migration's re-run path is genuinely idempotent or that its error text contains 'duplicate column name', otherwise this could break production rollout.

[protostatis]

The if not update_existing: return -1 early-return now sits inside with sqlite_transaction(...), which means an empty transaction is COMMITTED on the 'already exists' path. It's not harmful (nothing was written), but it's a slight misuse of the helper's contract — the intent reads as 'rollback/skip', yet it commits. Consider resolving the existence check before opening the write transaction, or restructuring so the -1 return happens outside the context manager for clarity.

[protostatis]

.lower() is now applied to settings.embedding_backend but the subsequent comparison if cfg_backend == 'openrouter' and the else ValueError use the lowered value, which is correct. However the previous fallback path also accepted any non-'openrouter' value as 'local'; now unknown values raise ValueError. Confirm no existing deployment has a non-lowercase or typo'd EMBEDDING_BACKEND (e.g. 'OpenRouter' or 'open_router') that will now crash startup rather than silently defaulting — the fail-closed intent is right, but the config collision risk is worth a doc/validation note.

[protostatis]

The coin_filter string is interpolated into the query via f-string, but it only injects static SQL fragments and the coin value itself stays parameterized via ?, so there is no injection risk. Just confirm the params order remains [cutoff, coin?, fear_greed_cutoff] if the coin-filter branch ever adds additional placeholders in future edits, as the positional binding is order-sensitive and easy to break during refactoring.