feat(storage): preserve retention evidence for later evaluation#333
feat(storage): preserve retention evidence for later evaluation#333wenchanghan wants to merge 13 commits into
Conversation
Preserve every retention-trimmed SQLite row as append-only JSONL, including cascade dependents, while keeping the feature default-off and failing closed on archive errors. Document the evidence lifecycle and cover the archive contract with integration tests.
Hold SQLite's writer guard across row selection, archive writes, and deletion so concurrent children cannot be removed without evidence. Cover both the race and the automatic over-limit cleanup path.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds opt-in JSONL archiving before retention deletion, SQLite transaction and row-fetch hooks, cascade archival, configurable archive directories, bounded cleanup batches, and integration coverage. It also adds bounded retrieved-learning snapshots and omits null citation fields during interaction persistence. ChangesRetention archive
Retrieved-learning snapshots
Citation serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RetentionCleanup
participant SQLiteDeletionMixin
participant retention_archive
participant SQLiteDatabase
RetentionCleanup->>SQLiteDeletionMixin: select oldest retention keys
SQLiteDeletionMixin->>SQLiteDatabase: fetch target and cascade rows
RetentionCleanup->>retention_archive: append_archive_batch(...)
retention_archive-->>RetentionCleanup: enforce archive max_bytes
RetentionCleanup->>SQLiteDeletionMixin: delete retention rows
SQLiteDeletionMixin->>SQLiteDatabase: complete guarded transaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cap total JSONL storage at 10 GiB by default and stop trimming when full, while batching archive reads and fencing cleanup across SQLite connections. Add high-volume and ceiling-path coverage, and remove client-specific design docs from the OSS change.
|
Review findings (critical pass; the archive suite is green locally — 11/11 in 1. Consider splitting the snapshot feature into its own PR. 2. Accepted payload bytes are unbounded. 3. Flag-on cleanup holds the global write lock across file I/O — costly at default caps. 4. Simplification opportunities.
5. Gaps.
Also verified good in this pass: fail-closed holds end-to-end (fetch errors re-raise as 🤖 Generated with Claude Code |
|
Incorporated the actionable review items in
I kept the warning at error severity because the agreed product behavior is a loud operational signal once the unbounded JSONL archive crosses its threshold. I also kept the snapshot commit in this connected PR set: capturing injection-time content is required now so the follow-up can focus solely on reading JSONL into the evaluator. Full suite: 4,751 passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
reflexio/server/services/storage/sqlite_storage/base/_deletion.py (1)
69-103: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winApply the safe identifier quoting consistently across all statements.
escaped_tableis computed and correctly doubled-and-quoted, but it is only used in thePRAGMA(Line 73). TheSELECTon Lines 86 and 103 interpolate the rawtable_name, and theWHEREclauses on Lines 87 and 97 interpolate unquotedkey_columns, while the projectedcolumnsare quoted. Inputs come from trustedRetentionTargetconfig today, so there is no live injection, but the quoting invariant is only half-enforced and will silently break if any table/key identifier ever becomes dynamic. Reusingescaped_tableand quotingkey_columnsthe same way ascolumn_namesmakes the invariant hold and clears the OpenGrep S608 finding on Lines 72-74.🛡️ Suggested consistent quoting
columns_sql = ", ".join(quoted_columns) + quoted_keys = [ + '"' + column.replace('"', '""') + '"' for column in key_columns + ] if len(key_columns) == 1: rows = self._select_in_chunks( - f"SELECT {columns_sql} FROM {table_name} " # noqa: S608 - f"WHERE {key_columns[0]} IN ({{placeholders}})", + f'SELECT {columns_sql} FROM "{escaped_table}" ' # noqa: S608 + f"WHERE {quoted_keys[0]} IN ({{placeholders}})", [key[0] for key in keys], ) return [dict(row) for row in rows] params_per_key = len(key_columns) rows_per_chunk = max(1, RETENTION_DELETE_CHUNK // params_per_key) rows: list[Any] = [] for key_chunk in chunked(keys, rows_per_chunk): where = " OR ".join( - "(" + " AND ".join(f"{column} = ?" for column in key_columns) + ")" + "(" + " AND ".join(f"{column} = ?" for column in quoted_keys) + ")" for _ in key_chunk ) params = [value for key in key_chunk for value in key] rows.extend( self.conn.execute( - f"SELECT {columns_sql} FROM {table_name} WHERE {where}", # noqa: S608 + f'SELECT {columns_sql} FROM "{escaped_table}" WHERE {where}', # noqa: S608 params, ).fetchall() )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/base/_deletion.py` around lines 69 - 103, Apply identifier quoting consistently in the deletion query flow: reuse the escaped, quoted table identifier derived for the PRAGMA instead of interpolating raw table_name in both SELECT statements, and create escaped/quoted key-column identifiers before building the single-key and composite-key WHERE clauses. Update the SQL construction around _select_in_chunks and the composite-key rows loop while preserving their existing parameter binding and chunking behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@reflexio/server/services/storage/sqlite_storage/base/_deletion.py`:
- Around line 69-103: Apply identifier quoting consistently in the deletion
query flow: reuse the escaped, quoted table identifier derived for the PRAGMA
instead of interpolating raw table_name in both SELECT statements, and create
escaped/quoted key-column identifiers before building the single-key and
composite-key WHERE clauses. Update the SQL construction around
_select_in_chunks and the composite-key rows loop while preserving their
existing parameter binding and chunking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 901abca1-60a3-4d2f-afba-bbe0aa732a80
📒 Files selected for processing (11)
README.mdreflexio/models/api_schema/domain/entities.pyreflexio/server/services/generation_service.pyreflexio/server/services/storage/retention_archive.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/sqlite_storage/base/_deletion.pyreflexio/server/services/storage/sqlite_storage/profiles/_interaction_store.pytests/models/api_schema/test_input_size_bounds.pytests/server/api_endpoints/test_api_routes.pytests/server/services/storage/test_retention_archive.pytests/server/services/storage/test_storage_contract_retrieved_learning_evals.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- reflexio/server/services/storage/retention_mixin.py
wenchanghan
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
reflexio/server/services/storage/retention_archive.py (1)
113-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the dead archive-state tracking
_warned_archive_dirshas no readers anywhere in the repo, andprojected_bytes += len(encoded_line)has no effect after the write loop. Remove both unless a future warning path still needs the set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/retention_archive.py` around lines 113 - 148, Remove the unused _warned_archive_dirs tracking, including its discard/add operations, unless an existing warning path reads it. In the archive write block around the retention archive flow, remove the ineffective projected_bytes += len(encoded_line) update after each write while preserving the write and ceiling-check behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@reflexio/server/services/storage/retention_archive.py`:
- Around line 113-148: Remove the unused _warned_archive_dirs tracking,
including its discard/add operations, unless an existing warning path reads it.
In the archive write block around the retention archive flow, remove the
ineffective projected_bytes += len(encoded_line) update after each write while
preserving the write and ceiling-check behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92bc8255-a415-454f-ada1-628e5857a25f
📒 Files selected for processing (3)
reflexio/server/services/storage/retention_archive.pyreflexio/server/services/storage/retention_mixin.pytests/server/services/storage/test_retention_archive.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/storage/retention_archive.py`:
- Around line 98-105: Update the archive entry flow around the segment creation
and cleanup logic to sweep stale .tmp files in archive_dir before calculating
size or evicting archived segments. Ensure orphaned temporary files are removed
or included in retention accounting so repeated crashes cannot bypass max_bytes,
while preserving the existing atomic write-and-replace behavior.
- Around line 107-123: Update the archive eviction loop over segments to
tolerate files disappearing between discovery, stat, and unlink. Handle
missing-file errors around the affected operations as no-ops, continuing the
sweep without incrementing eviction counters or failing the retention pass,
while preserving existing behavior for other filesystem errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7905c8de-82a2-42f2-a70b-da701202f787
📒 Files selected for processing (4)
README.mdreflexio/server/services/storage/retention_archive.pyreflexio/server/services/storage/retention_mixin.pytests/server/services/storage/test_retention_archive.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- reflexio/server/services/storage/retention_mixin.py
- tests/server/services/storage/test_retention_archive.py
|
Re-review of the follow-up commits ( Prior findings all addressed:
The revision also changes two policies rather than just fixing findings; both are implemented and documented consistently, but they invert the earlier contract, so flagging them explicitly for reviewers:
Two minor residuals, neither blocking:
Verified locally at The snapshot schema remains bundled with the archive change — previously noted, maintainers' call. Otherwise ready for lead review from my side. 🤖 Generated with Claude Code |
|
Final archive-contract clarification:
|
|
Closing — superseded by a direction change |
1 participant
Summary
Reflexio bounds live SQLite tables by deleting old rows. This PR optionally preserves those rows in a bounded JSONL FIFO so older sessions remain available for later evaluation without allowing either storage path to grow forever.
Flow
flowchart LR A["SQLite selects up to 1,000 old target rows"] --> B{"Does the batch fit?"} B -->|Yes| C["Write one JSONL segment"] --> D["Delete the same live rows"] B -->|No| E["Split into smaller complete batches"] --> C C --> F["If over the ceiling, evict oldest segments"] F --> G["Next PR: read live + JSONL into evaluator"]Archive contract
REFLEXIO_RETENTION_ARCHIVE=1REFLEXIO_RETENTION_ARCHIVE_MAX_BYTES=10737418240Archive files always live in an
archivedirectory beside the SQLite database—for example,~/.reflexio/data/archivebeside the default database.Each archive-enabled SQLite transaction selects at most 1,000 retention-target rows. This is a work-size bound, not a failure threshold. A timestamped
*.jsonlsegment contains those rows and any related rows deleted with them. For example, deleting an oldrequestsrow also deletes and archives theinteractionsrows that reference itsrequest_id; many targets have no related rows and remain single-table batches.If a batch expands beyond the byte ceiling, Reflexio recursively splits it into smaller complete segments instead of dropping the entire batch. FIFO then retains the newest complete groups that fit. Only one indivisible target plus its related rows can be skipped if that group alone exceeds the entire archive ceiling. Archive I/O failures are also logged. In all cases, live retention continues so learning and application behavior remain available and SQLite stays bounded.
Example records
One segment may contain several tables; every line identifies its table explicitly.
{"table":"requests","archived_at":1783792800,"row":{"request_id":"req-123","user_id":"project-7","session_id":"session-42","created_at":1783789200,"source":"agent","agent_version":"v1"}} {"table":"interactions","archived_at":1783792800,"row":{"interaction_id":987,"request_id":"req-123","role":"Assistant","content":"Use pathlib here.","retrieved_learnings":"[{\"kind\":\"user_playbook\",\"learning_id\":\"11\",\"snapshot\":{\"title\":\"Filesystem discipline\",\"content\":\"Use pathlib for filesystem work.\",\"trigger\":\"writing Python filesystem code\",\"rationale\":\"Path objects are easier to compose.\"}}]"}}Why the complete interaction row is retained
This is evaluation evidence, not an additional application log. Existing evaluation paths use more than message text:
shadow_content;retrieved_learnings.Dropping those fields here would make some retroactive evaluations impossible. The large vector
embeddingcolumn is excluded because no evaluator needs it. Tests enforce “all interaction columns except embedding.”The learning snapshot contains the injection-time source fields associated with that response. It lets a later evaluator avoid substituting a learning that has since been edited.
What comes next
The evaluator follow-up only needs to:
Verification
Boundaries