fix(phlower): chunk retention purge so it stops starving the flush loop#30
Conversation
|
@codex review |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99b45b91c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while remaining > 0: | ||
| await asyncio.sleep(_PURGE_CHUNK_PAUSE_SECONDS) | ||
| remaining = await loop.run_in_executor( | ||
| None, sqlite_store.vacuum_step, config.sqlite_vacuum_chunk_pages | ||
| ) |
There was a problem hiding this comment.
When upgrading a database that was created before auto_vacuum=INCREMENTAL, SQLite leaves PRAGMA auto_vacuum at NONE, so vacuum_step() can return the same positive freelist_count forever after this purge frees pages. This loop has no progress check, so the purge task can run indefinitely and never reach checkpointing, cached stats refresh, or later emergency-retention passes; please detect unsupported incremental vacuum or break/fallback when remaining does not decrease.
Useful? React with 👍 / 👎.
| if cur.rowcount == 0: | ||
| # Drained (pages already freed by earlier deletes) — drop is | ||
| # cheap. Fall through so invocations gets worked this step too. | ||
| self._conn.execute(f"DROP TABLE IF EXISTS {det_tbl}") |
There was a problem hiding this comment.
When the details table has been drained, this drops invocation_details_* while invocations_* remains until a later chunk. During that window _read_tables() still assumes every invocation partition has a matching details table and builds LEFT JOIN invocation_details_* for lookup_task_id, list_by_task, and search, so those reads can raise sqlite3.OperationalError: no such table even for recent queries because the UNION spans all partitions; keep an empty details table until the invocation table is dropped or make table discovery verify the details table exists.
Useful? React with 👍 / 👎.
| # ~7M-row partition pair plus an unbounded ``incremental_vacuum`` under | ||
| # one lock hold (~30 min on EBS): the flush loop starved, its write- | ||
| # behind buffer overflowed, and ~1.4M records were dropped every purge | ||
| # night while RSS stepped up and eventually OOM-killed the pod. The |
There was a problem hiding this comment.
AGENTS.md says public code comments must not reference internal infrastructure or deployment-incident details. This new comment includes exact row counts, duration, EBS, RSS/OOM, and pod-specific language, which leaks internal operational context into the open-source repo; please rewrite it generically, e.g. describing large databases/high-throughput environments without incident-specific numbers or infrastructure names.
Useful? React with 👍 / 👎.
On a large database the retention purge could hold the SQLite write lock for many minutes: with auto_vacuum=INCREMENTAL both DROP TABLE of a day partition and the follow-up unbounded incremental_vacuum touch every freed page, and everything ran under a single lock hold. The flush loop starved for the whole pass, its write-behind buffer overflowed (silently dropping records), and memory ballooned with each purge until the process could get killed. Replace the single-hold purge with small serialized primitives: - purge_partition_step deletes up to SQLITE_PURGE_CHUNK_ROWS (50K) per call; the drained details table is kept until the end because reads LEFT JOIN it, then both tables drop in one transaction. A crash mid-purge resumes on the next tick since partition discovery keys off invocations_%. - vacuum_step reclaims up to SQLITE_VACUUM_CHUNK_PAGES (10K) per call and returns the remaining freelist; it bails out when auto_vacuum is not actually INCREMENTAL (pre-pragma files) so the trickle cannot spin forever, and the caller breaks when a step makes no progress. - app.py paces the chunks with a 250 ms sleep so flush_batch always gets the lock in between. The emergency disk-cap path re-checks disk usage only after the vacuum trickle — chunked deletes only shrink the file once pages are returned, so rechecking earlier would spuriously halve retention again. Also adds the repo's first tests (pytest against real tmp SQLite files) and a minimal CI workflow to run them. Generated-By: PostHog Code Task-Id: 41462379-8751-40f7-b68c-dfe5dc89d1b8
webjunkie
force-pushed
the
posthog-code/fix-phlower-chunked-purge
branch
from
99b45b9 to
cd2bdfb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd2bdfbfec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| remaining = await loop.run_in_executor( | ||
| None, sqlite_store.vacuum_step, config.sqlite_vacuum_chunk_pages | ||
| ) |
There was a problem hiding this comment.
If vacuum_step raises during incremental vacuum, for example from SQLite I/O or disk-full errors, this await lets the exception escape _purge_partitions_chunked; the _sqlite_purge_loop task is created without supervision, so later retention, checkpoint, metadata, and health-stat passes stop running. The previous _reclaim_free_pages() path caught and logged vacuum failures, so the chunked vacuum calls should preserve that behavior.
Useful? React with 👍 / 👎.
| break | ||
| await asyncio.sleep(_PURGE_CHUNK_PAUSE_SECONDS) | ||
|
|
||
| # Legacy tables: cheap wholesale drop once expired (gone in prod). |
There was a problem hiding this comment.
AGENTS.md says public commits and code comments must not reference internal infrastructure or deployment details; gone in prod exposes an internal production-state detail in this public repo. Please keep the purge explanation generic here and in the matching SQLiteStore docstring rather than referring to production.
Useful? React with 👍 / 👎.
…s current (#31) CLAUDE.md is now a symlink to AGENTS.md so Claude Code reads the same guidance as other agents — one file to maintain. While at it, checked AGENTS.md against the current code: the design brief (tokens, layout dimensions, typography, SSE events, data layer) and the public-repo guidelines all still match, so they're untouched. Two gaps filled: - a "Keeping docs current" section — doc updates belong in the same PR as the change (env vars -> README config table, persistence changes -> README architecture section, design changes -> the brief itself) - a short "Tests" section, since the repo gained its first test suite and CI in #30 FYI the README itself has some drift (config table missing a few newer env vars, SQLite section predates partitioning) — that's a separate pass, currently held back to avoid colliding with in-flight work touching the same table. --- *Created with [PostHog Code](https://posthog.com/code?ref=pr)*
2 participants
The hourly retention purge does DROP TABLE of expired day partitions plus an unbounded incremental_vacuum under one SQLite write-lock hold. With auto_vacuum=INCREMENTAL both are O(freed pages), so on a large database that's a multi-minute hold. The flush loop shares that lock: it starves for the whole pass, its write-behind buffer overflows and silently drops records, and memory balloons with each purge until the process can get killed.
What changed
The purge is now a paced trickle of small serialized steps: chunked deletes per expired partition, then chunked incremental_vacuum until the freelist is empty, with a 250 ms pause between chunks so the flush loop always gets the lock. Chunk sizes are tunable via SQLITE_PURGE_CHUNK_ROWS and SQLITE_VACUUM_CHUNK_PAGES.
Details that matter:
Tests
First tests in the repo: chunk-resume, cutoff boundary, reads during a partial purge, vacuum shrink + no-op guards, end-to-end. Plus a minimal tests.yml workflow (pinned actions, contents: read).
Created with PostHog Code