Repository navigation
fix(ingestion): a stale webhook replay must not zero out a document's active version - #46
Conversation
… active version Dedupe only checked the ACTIVE row: a replayed old payload deactivated the newer version while the duplicate-hash insert silently no-opped, committing success with zero active versions. Any version row matching the hash now makes the delivery a skipped duplicate. Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
…ran Rathore <180254320+charan-rathore@users.noreply.github.com> Add a method to fetch document version by hash payload.
…ctive versions Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Add regression tests for stale webhook replays in Postgres.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc55a1d8bb
ℹ️ 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".
| import psycopg | ||
| import pytest | ||
|
|
||
| pgserver = pytest.importorskip("pgserver", reason="pgserver not installed") |
There was a problem hiding this comment.
Install pgserver so the regression test actually runs
In a standard project installation this line skips the entire new module because pgserver is absent from rag-platform/pyproject.toml and no workflow installs it. Running the targeted suite here produced 32 passed, 1 skipped, so the stale-replay regression is not enforced by the repository's declared test setup; add pgserver to a test extra and run that extra in CI rather than silently skipping the only end-to-end coverage.
Useful? React with 👍 / 👎.
| existing = repository.get_version_by_hash( | ||
| document.document_id, version.hash_payload, conn=conn | ||
| ) | ||
| if existing is not None: |
There was a problem hiding this comment.
Report inactive historical hashes as duplicates
When the incoming hash belongs to an inactive version, the preceding ingestion_dedupe_checked event has already recorded is_duplicate=False because it compares only with the active row, but this new lookup then classifies the same request as a duplicate and emits skipped_no_change. Metrics or audits consuming the documented is_duplicate field therefore undercount precisely the stale replays handled by this fix; perform this lookup before emitting the dedupe event and include its result.
Useful? React with 👍 / 👎.
| existing = repository.get_version_by_hash( | ||
| document.document_id, version.hash_payload, conn=conn | ||
| ) |
There was a problem hiding this comment.
Apply the stale-hash guard during stored-payload reprocessing
The public reprocess_github_payloads_to_postgres path still performs only the active-hash comparison at lines 700-714. For a document with multiple stored payloads, list_payloads_for_document returns the newest first, so it skips the current payload and then processes an older hash by deactivating the current version; insert_versions silently ignores that already-existing hash and the operation commits with zero active versions. Route reprocessing through this any-version lookup or the shared ingestion routine as well, otherwise the same corruption fixed for live webhooks remains guaranteed when reprocessing a versioned document.
Useful? React with 👍 / 👎.
| existing = repository.get_version_by_hash( | ||
| document.document_id, version.hash_payload, conn=conn | ||
| ) |
There was a problem hiding this comment.
Serialize the hash check with the version transition
When two workers ingest the same previously unseen payload concurrently, both transactions can pass this lookup; if the first commits the new active version after the second lookup but before the second calls deactivate_active_version, the second transaction deactivates that newly inserted version and its own insert_versions then hits ON CONFLICT (document_id, hash_payload) DO NOTHING, again committing with zero active versions. Lock the document or active-version row across the lookup and transition, or make the transition atomic, so concurrent at-least-once deliveries cannot reproduce the corruption this patch is intended to prevent.
Useful? React with 👍 / 👎.
…against stale hashes Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Refactor duplicate check logic to simplify handling of active and existing versions. Remove redundant checks for stale replays and streamline the deduplication process.
|
Deployment failed for project intellirag-live-own-track with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
Deployment failed for project irag-fix-0904 with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
|
Deployment failed for project intellirag with the following error: Learn More: https://vercel.com/own-track?upgradeToPro=build-rate-limit |
…ff-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added a lock_document method to handle row-locking for document transactions, preventing concurrent version conflicts.
…sion active Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com> Added a test to ensure reprocessing stored payloads keeps the newer version active.
…haran Rathore <180254320+charan-rathore@users.noreply.github.com> Added test dependencies for pytest and pgserver.
…haran Rathore <180254320+charan-rathore@users.noreply.github.com>
|
@codex review All four findings addressed:
RED/GREEN on real Postgres: the reprocess regression fails without the guard (0 != 1 active versions) and passes with it; workers+connectors 36 passed. Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Problem
Delivery is at-least-once, so an old webhook payload can arrive after a newer version is already active. The dedupe check only compared against the active row, so the replay was treated as an update:
_prepare_version_transitiondeactivates the newer active version (hash B),insert_versionsrunsON CONFLICT (document_id, hash_payload) DO NOTHING, so the duplicate-hash row (hash A, already present as the inactive v1) is silently skipped,Proof (real Postgres)
New test runs the real
_ingest_single_payloadagainst a real Postgres (pgserver) with the real repositories and migrations 001-005 applied:Verified RED on main (AssertionError: 0 != 1, "document must keep exactly one active version") and GREEN with the fix.
Fix
A payload hash that exists on any version row - active or not - is a duplicate delivery, not an update:
document_repository.get_version_by_hashlooks the hash up, and the pipeline returnsskippedbefore any deactivation. Replays are now idempotent and the newer version always stays active.Note: documents already in the zero-active state are not repaired by this change; re-ingesting a genuinely new payload for one now fails loudly on the version-index unique constraint instead of silently corrupting further - a repair pass is a separate follow-up.
Tests
apps/workers/app/tests/test_ingestion_replay_realdb.py(skips cleanly without pgserver/Postgres): the stale-replay scenario above, plus a control that re-ingesting the current payload still skips.apps/workers+libs/connectorssuites: 35 passed.Signed-off-by: Charan Rathore 180254320+charan-rathore@users.noreply.github.com