feat(files): persist + expose indexation timestamp (files.created_at) - #529
Merged
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds an Changesindexed_at Timestamp Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Ahmath-Gadji
requested changes
Jun 22, 2026
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
Collaborator
There was a problem hiding this comment.
Request: expose the indexation timestamp as indexed_at, not created_at
Thanks for tracking down why the "Indexed" column was empty — the diagnosis and the idempotent migration are spot-on. One blocking concern with the key name, though.
created_at is a reserved, client-supplied temporal field, not a free key we can repurpose. Per the API docs (API.mdx#L93-L102):
created_at: ISO 8601 format date of when the file was created
created_atis provided by the client in the metadata of the file during upload.
It's the user's document date (used for temporal-aware search/filtering), not the system's insert time. Placing the new column after the **metadata spread in _row_to_dict (openrag/services/persistence/document_repo.py:562) silently shadows that client value with the indexation time in the file-listing surface.
The two concepts should stay separate. Please expose the indexation timestamp as indexed_at instead:
- It's already the codebase's intended key for this — the UI reads
file.indexed_at ?? file.created_at(extern/indexer-ui/src/routes/indexer/partition/[partition]/+page.svelte:252,298), soindexed_atfills the "Indexed" column directly. - It leaves the reserved
created_at(client temporal field) intact, socreated_atsorting (+page.svelte:70) and temporal filtering keep working on the document date.
Concretely: rename the new column files.created_at → files.indexed_at across the migration, schema.py, and the _row_to_dict key. The rest of the change (server default, idempotent guard, INSERTs relying on the default) carries over unchanged.
Everything else in the PR looks good and CI is green — just this naming.
Ahmath-Gadji
approved these changes
Jun 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/services/workers/test_indexer_worker.py (1)
267-267: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert timezone awareness explicitly for
indexed_at.Current checks only validate
datetimetype (and equality once). A naive datetime would still pass, which weakens protection of the timezone-aware timestamp contract.Proposed test hardening
- assert isinstance(add_call.pop("indexed_at"), datetime) + add_indexed_at = add_call.pop("indexed_at") + assert isinstance(add_indexed_at, datetime) + assert add_indexed_at.tzinfo is not None and add_indexed_at.utcoffset() is not None @@ store_indexed_at = store.calls[0][2] catalog_indexed_at = repo.add_calls[0]["indexed_at"] assert isinstance(store_indexed_at, datetime) + assert store_indexed_at.tzinfo is not None and store_indexed_at.utcoffset() is not None + assert catalog_indexed_at.tzinfo is not None and catalog_indexed_at.utcoffset() is not None # The store and the catalog must receive the very same timestamp object/value. assert store_indexed_at == catalog_indexed_at @@ - assert isinstance(update_call.pop("indexed_at"), datetime) + update_indexed_at = update_call.pop("indexed_at") + assert isinstance(update_indexed_at, datetime) + assert update_indexed_at.tzinfo is not None and update_indexed_at.utcoffset() is not NoneAlso applies to: 299-301, 353-353
🤖 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 `@tests/unit/services/workers/test_indexer_worker.py` at line 267, The assertion for indexed_at at line 267 only validates that it is a datetime instance, but does not check for timezone awareness. A naive datetime without tzinfo would still pass this check, weakening the validation of the timezone-aware timestamp contract. Add an explicit assertion to verify that the indexed_at datetime object is timezone-aware by checking that its tzinfo attribute is not None. Apply the same fix to the similar assertions at lines 299-301 and 353 as indicated in the comment.
🤖 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 `@tests/unit/services/workers/test_indexer_worker.py`:
- Line 267: The assertion for indexed_at at line 267 only validates that it is a
datetime instance, but does not check for timezone awareness. A naive datetime
without tzinfo would still pass this check, weakening the validation of the
timezone-aware timestamp contract. Add an explicit assertion to verify that the
indexed_at datetime object is timezone-aware by checking that its tzinfo
attribute is not None. Apply the same fix to the similar assertions at lines
299-301 and 353 as indicated in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 347122dd-86c3-40bb-ab43-c0400c1b1734
📒 Files selected for processing (12)
openrag/core/vector_stores/vector_store.pyopenrag/services/persistence/document_repo.pyopenrag/services/persistence/migrations/alembic/versions/b7c1d2e3f4a5_add_files_indexed_at.pyopenrag/services/persistence/schema.pyopenrag/services/storage/milvus_store.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/stages/store.pytests/unit/conftest.pytests/unit/services/workers/stages/test_pipeline_stages.pytests/unit/services/workers/test_batch_ingest.pytests/unit/services/workers/test_indexer_worker.pytests/unit/services/workers/test_pipeline_builder.py
✅ Files skipped from review due to trivial changes (2)
- openrag/services/persistence/migrations/alembic/versions/b7c1d2e3f4a5_add_files_indexed_at.py
- openrag/services/persistence/schema.py
The documents list reads from the Postgres files catalog, which had no timestamp column, so the admin UI "Indexed" column was always empty — the index time existed only in Milvus chunk metadata (shown in the file detail). Add a files.created_at column (timezone-aware, server_default now()), surface it in the file-listing dict (_row_to_dict, placed after the metadata spread so the column wins), and add an idempotent migration. Existing rows backfill to the migration run time via the server default; newly indexed files get their true insert time. The admin UI already renders this column.
created_at is a reserved, client-supplied temporal field (provided in file_metadata at upload time for time-based filtering), so the indexation timestamp must not reuse that key. Rename the new files column, its migration, and the _row_to_dict surface from created_at to indexed_at. This matches the existing chunk-level indexed_at field and the admin UI, which reads `indexed_at ?? created_at`, so the document list's Indexed column populates while the client created_at stays available for filtering.
Ahmath-Gadji
force-pushed
the
fix/files-created-at
branch
from
June 22, 2026 12:52
0f0c925 to
4f5c4ad
Compare
…s row The Milvus upsert and the Postgres catalog write each generated their own now(), so a file's chunk indexed_at and its files.indexed_at could drift. Mint a single timestamp in process_file and thread it to both sinks: the store stage forwards it to VectorStore.upsert (stamped on every chunk) and _write_catalog_record passes it to the files INSERT/UPDATE. Both arguments default to None, falling back to now()/the server default, so existing callers (direct upsert, copy/restore, create_document) are unaffected. Re-index (replace) refreshes indexed_at to match the re-upserted chunks.
…heads Rebasing onto refactor/hexagonal brought in the topic_tags migration (b7c8d9e0f1a2), which also descends from 06dd2101ea3a. Two siblings off the same parent give Alembic multiple heads, so `alembic upgrade head` fails at startup and the ServiceContainer never initializes (every request 500s). Re-parent indexed_at (b7c1d2e3f4a5) onto b7c8d9e0f1a2 for a single linear head.
Ahmath-Gadji
force-pushed
the
fix/files-created-at
branch
from
June 22, 2026 13:20
4f5c4ad to
8a34772
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Ahmath-Gadji
added a commit
that referenced
this pull request
Jun 22, 2026
…eanup Resolves the indexer_actor.py conflict between the parser refactor and #529: keeps the async, file_id-required _load_document from the refactor and unions in #529's shared indexed_at threading (process_file reads row['indexed_at'] after pipeline.run and passes it to _write_catalog_record, which forwards it to both the add/update catalog writes). All other #529 changes (store stage, vector_store, milvus_store, document_repo, schema, migration) apply cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The admin UI documents list has an "Indexed" column, but it's always empty. The list is built from the Postgres
filescatalog (SELECT * FROM files→_row_to_dict), and that table has no timestamp column — the index time exists only in the Milvus chunk metadata (which is why the file detail view showscreated_at/indexed_atbut the list can't).Fix
files.created_atcolumn —DateTime(timezone=True),server_default now(),NOT NULL— to the model (schema.py)._row_to_dict(placed after the**metadataspread so the column value always wins).column_existsguard, both directions), consistent with the repo's create_all-then-migrate policy.The file INSERT doesn't list
created_at, so the server default sets it at insert time = the indexation time. No UI change needed — the existing "Indexed" column readsindexed_at ?? created_at.Caveat
Existing files have no recorded index time, so the
server_defaultbackfills them to the migration run time; newly indexed files get their true insert time.Tests
Persistence + partition-service unit suites green (66 passed); ruff clean. Both
_row_to_dictcallers useSELECT *, so the new column is always present.Summary by CodeRabbit
New Features
Chores
indexed_atcolumn with a default for existing rows.