Repository navigation
Persist published takeaways in the system database - #55
Conversation
The analysis costs minutes of model time and lived only in the process, so every deploy today blanked the card. A finished run is now upserted to insights_takeaways (one row per org database: generated_at, model, seconds, and the rendered payload), and an engine with nothing in memory reads it back on the first ask. A saved analysis younger than the six-hour TTL is served as is; an older one is shown while a new run replaces it. Writes and reads are best effort: a storage failure costs persistence, not the analysis.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| now = time.time() | ||
| with self._lock: | ||
| entry = self._takeaways.get(database_id) | ||
| if entry is None and self.store is not None: |
There was a problem hiding this comment.
nit: Cache the absence of a saved row too, for example with a sentinel in _takeaways or a _checked set. (not blocking)
When an org has no row yet, _takeaways stays empty until the first run finishes. Until then, each page poll runs a synchronous sysdb query on the request thread. A first run takes minutes, and the page polls every ten seconds. A failing read also repeats on every poll.
| rows = self.store.rows( | ||
| f"SELECT generated_at, model, seconds, payload " | ||
| f"FROM {core.SYSTEM_CATALOG}.public.{self.TABLE} " | ||
| f"WHERE database_id = '{database_id}'") |
There was a problem hiding this comment.
nit: Quote database_id the way server.sql_str does: .replace("'", "''"). (not blocking)
database_id comes from the server's org record today, so this query is not injectable now. StorePool.detail validates its interpolated id with SESSION_ID_RE before use. This query has no equivalent guard.
| data = parse_takeaways(answer) | ||
| entry = {"at": time.time(), "generatedAt": _now_iso(), "data": data, | ||
| "steps": steps, "seconds": round(time.time() - started)} | ||
| self._save(database_id, entry) |
There was a problem hiding this comment.
nit: In the except branch below, keep the previous successful entry when one exists. Store the error alongside that entry instead of replacing it. (not blocking)
Now that a stale saved analysis is loaded and refreshed, a failed refresh replaces it in memory with the error entry. The card then shows an error instead of the last published takeaways. The saved row is still intact, but this process no longer serves it. The PR description says a stale analysis "is shown while a new run replaces it". A failed run never replaces it.
1 participant
Takeaways lived only in the server process, so every deploy (several today) blanked the Insights card and started a new model run on the next view.
insights_takeaways, one row per org database:generated_at,model,seconds,payload(the highlights and the queries, as the page renders them). Created at boot like the other system tables.Test: a canned run through one engine is read back by a fresh engine over the same fake database without regenerating. All suites pass.