fix(knowledge): stabilize Azure Search file lifecycle - #336
Conversation
Signed-off-by: Harmke Alkemade <halkemade@nvidia.com>
|
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:
WalkthroughAzure AI Search ingestion now requires stable manifest and chunk visibility before job completion. Deterministic chunk IDs support rollback and deletion by expected counts. Collection and file deletion wait for confirmed removal, while tests simulate delayed and stale visibility. ChangesAzure AI Search consistency and lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AzureAISearchIngestor
participant AzureAISearch
participant JobState
AzureAISearchIngestor->>AzureAISearch: Upload manifests and deterministic chunks
AzureAISearchIngestor->>AzureAISearch: Poll stable terminal visibility
AzureAISearch-->>AzureAISearchIngestor: Return terminal manifests and chunk counts
AzureAISearchIngestor->>JobState: Mark job COMPLETED
AzureAISearchIngestor->>AzureAISearch: Delete deterministic documents on failure
AzureAISearchIngestor->>JobState: Mark files and job FAILED
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
1356-1400: 🧹 Nitpick | 🔵 TrivialDeletion confirmation now costs strictly more latency than before.
delete_filepreviously waited once for the manifest state; it now waits forstable_reads=3consecutive zero-count reads across manifest+chunks before proceeding to summary/timestamp updates and bookkeeping. This is the right correctness tradeoff (per the earliertest_delete_timeout_preserves_retryable_bookkeepingbehavior) but is worth calling out operationally: synchronous callers ofdelete_filewill now observe noticeably higher latency per delete, proportional to_CONSISTENCY_ATTEMPTS * _CONSISTENCY_DELAY_SECONDSgated by the 3-in-a-row requirement.🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py` around lines 1356 - 1400, Document the increased synchronous deletion latency around delete_file and its _wait_for_search_count call, including the _CONSISTENCY_STABLE_READS requirement and retry timing. Preserve stable_reads=3 and the existing correctness behavior; do not reduce consistency checks or alter deletion bookkeeping.
🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Around line 879-901: The per-file sequential waits in _wait_for_job_visibility
multiply consistency latency by the number of files. Aggregate terminal file IDs
and perform the manifest visibility check and chunk-count visibility check
across the entire job using combined file-ID filters, while preserving each
file’s terminal-status validation and expected total chunk count semantics.
---
Outside diff comments:
In `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Around line 1356-1400: Document the increased synchronous deletion latency
around delete_file and its _wait_for_search_count call, including the
_CONSISTENCY_STABLE_READS requirement and retry timing. Preserve stable_reads=3
and the existing correctness behavior; do not reduce consistency checks or alter
deletion bookkeeping.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 5f1fe696-fbb5-4893-9437-06dc9955d06b
📒 Files selected for processing (2)
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/knowledge_layer_tests/test_azure_ai_search.pysources/knowledge_layer/src/azure_ai_search/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_azure_ai_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
🔇 Additional comments (8)
sources/knowledge_layer/src/azure_ai_search/adapter.py (6)
72-72: LGTM!Also applies to: 197-200
983-992: 🎯 Functional Correctness | 🏗️ Heavy liftJob can be marked FAILED while its files remain SUCCESS.
When
_wait_for_job_visibilityraises (finalize-wait timeout),_fail_jobonly updates job-levelstatus/error_message/completed_at— it never touchesfile_detailsorself._files. Files that already completed and had_update_file_progress(..., status=SUCCESS, ...)applied keep reportingFileStatus.SUCCESSviaget_file_status/list_files, even thoughget_job_statusreportsJobState.FAILED. Callers relying on job status to gate visibility of ingested files could be misled into thinking no data was ingested when it actually was (just not yet consistency-confirmed).Please confirm this divergence is the intended contract; if not, either propagate a distinct "completed but unconfirmed" signal or reconcile file-level state with the job failure.
1024-1024: LGTM!Also applies to: 1042-1057
1122-1140: LGTM!
1256-1278: LGTM!Also applies to: 1288-1298
625-673: 🚀 Performance & ScalabilityNo action needed
_CONSISTENCY_ATTEMPTS = 20leaves ample headroom forstable_reads = 3.tests/knowledge_layer_tests/test_azure_ai_search.py (2)
80-89: LGTM!Also applies to: 91-106, 108-123, 130-164
741-793: LGTM!Also applies to: 794-824, 1035-1055, 1084-1099
KyleZheng1284
left a comment
KyleZheng1284
left a comment
There was a problem hiding this comment.
I revalidated these lifecycle edge cases against the current PR head (13bda78). Suggestions are inline.
Signed-off-by: Harmke Alkemade <halkemade@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
1328-1343: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFence
submit_job()from collection deletion.
delete_collection()only writes_COLLECTION_DELETINGafter the_filessnapshot is taken, andsubmit_job()does not check for that state. A concurrent upload can still be accepted and finish after the child-document sweep, leaving documents behind or deleting the manifest while writes are still in flight. Move the deleting marker ahead of the snapshot or block new submissions on it; add a race regression test.🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py` around lines 1328 - 1343, The deletion flow in delete_collection must fence concurrent submit_job submissions before taking the files snapshot. Set or validate the _COLLECTION_DELETING marker before snapshotting, and update submit_job to reject new jobs for collections marked deleting, ensuring no uploads begin after deletion starts. Add a regression test covering the concurrent deletion/submission race and verifying no documents remain or are written after cleanup.Source: Path instructions
🤖 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 `@tests/knowledge_layer_tests/test_azure_ai_search.py`:
- Around line 918-940: Strengthen
test_response_loss_rolls_back_all_attempted_chunk_ids by configuring the fake
client’s visibility delay before processing the file, so uploaded chunks remain
queued rather than searchable. After the expected ServiceRequestError, assert
that attempted_ids are absent from both the visible documents store and the
client’s queued/pending store, using the fake client’s existing
visibility-related symbols.
- Around line 793-803: Strengthen the assertions in the test around job
finalization so rollback verifies removal of all Azure documents for the failed
job, not only absence of a successful manifest. Use the first job’s file_id from
jobs[0].file_details[0] and assert no document returned by the relevant Azure
Search listing/query retains that ID, while preserving the existing state and
successful_ids assertions.
---
Outside diff comments:
In `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Around line 1328-1343: The deletion flow in delete_collection must fence
concurrent submit_job submissions before taking the files snapshot. Set or
validate the _COLLECTION_DELETING marker before snapshotting, and update
submit_job to reject new jobs for collections marked deleting, ensuring no
uploads begin after deletion starts. Add a regression test covering the
concurrent deletion/submission race and verifying no documents remain or are
written after cleanup.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 044258f2-c262-4cff-8d3f-9c41f5610823
📒 Files selected for processing (2)
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/knowledge_layer_tests/test_azure_ai_search.pysources/knowledge_layer/src/azure_ai_search/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_azure_ai_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
🔇 Additional comments (2)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
625-672: LGTM!Also applies to: 879-918, 1001-1029, 1061-1114, 1159-1201, 1426-1471
tests/knowledge_layer_tests/test_azure_ai_search.py (1)
76-203: LGTM!Also applies to: 290-349, 667-770, 806-915
Signed-off-by: Harmke Alkemade <halkemade@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
1008-1010: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoll back chunks when success bookkeeping fails.
If
_process_filesucceeds but the subsequentSUCCESSmanifest/progress update throws, the catch resetschunks_createdto zero and marks the fileFAILED. Lines 1008-1010 then skip it, leaving its uploaded chunks searchable. Delete the known deterministic chunk IDs before overwriting the terminal state, and add a regression test for a failed success-manifest write.As per path instructions, review data-source changes for “external API error handling” and “deterministic tests.”
🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py` around lines 1008 - 1010, Update the failure-recovery path around _process_file and SUCCESS manifest/progress bookkeeping so that when the success update throws after chunks were uploaded, all known deterministic chunk IDs for that file are deleted before resetting chunks_created and marking the file FAILED. Do not rely on the get_job_status filtering loop to identify cleanup candidates, and add a deterministic regression test covering a failed success-manifest write and verifying the uploaded chunks are removed.Source: Path instructions
🤖 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.
Outside diff comments:
In `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Around line 1008-1010: Update the failure-recovery path around _process_file
and SUCCESS manifest/progress bookkeeping so that when the success update throws
after chunks were uploaded, all known deterministic chunk IDs for that file are
deleted before resetting chunks_created and marking the file FAILED. Do not rely
on the get_job_status filtering loop to identify cleanup candidates, and add a
deterministic regression test covering a failed success-manifest write and
verifying the uploaded chunks are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e23bf654-9959-447a-a74b-0db59ccf6280
📒 Files selected for processing (2)
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/knowledge_layer_tests/test_azure_ai_search.pysources/knowledge_layer/src/azure_ai_search/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_azure_ai_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
Signed-off-by: Harmke Alkemade <halkemade@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
1335-1335: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the deletion fence durable across ingestor instances.
_jobs_lockonly protects one process, while Azure Search reads are intentionally eventual. A secondAzureAISearchIngestorcan read the prior ACTIVE manifest after this DELETING upsert, accept a job, and publish chunks after this delete pass confirms zero children—leaving orphaned documents with no manifest retry anchor.
sources/knowledge_layer/src/azure_ai_search/adapter.py#L1335-L1335: use a shared durable lease/coordinator for submission and deletion; do not rely on local locking plus search visibility as mutual exclusion.tests/knowledge_layer_tests/test_azure_ai_search.py#L896-L927: use two ingestors sharing the fake backend, delay DELETING-manifest visibility, and assert the second instance cannot enqueue or publish documents.🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py` at line 1335, Make deletion coordination durable across AzureAISearchIngestor instances by replacing reliance on local _jobs_lock and eventual manifest visibility with a shared durable lease/coordinator covering both job submission and deletion around _write_collection_manifest. Update sources/knowledge_layer/src/azure_ai_search/adapter.py at lines 1335-1335 to enforce that coordination, and update tests/knowledge_layer_tests/test_azure_ai_search.py at lines 896-927 to use two ingestors sharing the fake backend, delay DELETING-manifest visibility, and assert the second ingestor cannot enqueue jobs or publish documents.
🤖 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.
Outside diff comments:
In `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Line 1335: Make deletion coordination durable across AzureAISearchIngestor
instances by replacing reliance on local _jobs_lock and eventual manifest
visibility with a shared durable lease/coordinator covering both job submission
and deletion around _write_collection_manifest. Update
sources/knowledge_layer/src/azure_ai_search/adapter.py at lines 1335-1335 to
enforce that coordination, and update
tests/knowledge_layer_tests/test_azure_ai_search.py at lines 896-927 to use two
ingestors sharing the fake backend, delay DELETING-manifest visibility, and
assert the second ingestor cannot enqueue jobs or publish documents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cc40fc6a-896d-44b8-86f9-ab11752c915d
📒 Files selected for processing (2)
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_azure_ai_search.py
🔇 Additional comments (9)
sources/knowledge_layer/src/azure_ai_search/adapter.py (6)
72-72: LGTM!Also applies to: 197-198, 625-672
838-840: LGTM!Also applies to: 882-921
968-1038: LGTM!Also applies to: 1070-1104
1168-1210: LGTM!
1446-1490: LGTM!
1115-1123: 🗄️ Data Integrity & IntegrationNo manifest rollback change needed.
_upload_documentsonly uploads chunk records; file and collection manifests are written on separate paths, and the rollback already targets only the attempted chunk IDs.> Likely an incorrect or invalid review comment.tests/knowledge_layer_tests/test_azure_ai_search.py (3)
76-84: LGTM!Also applies to: 86-109, 122-122, 146-203
745-806: LGTM!Also applies to: 860-893
953-986: LGTM!Also applies to: 1224-1241
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
1168-1192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude document contents in the stability condition.
Line 1183 treats identical ID sets as stable even when a file manifest’s
statusorchunk_countcontinues changing. Collection deletion can consequently derive incomplete deterministic chunk IDs from a stale manifest and confirm an empty collection before delayed chunks appear.Compare normalized selected documents—not only IDs—and add a deterministic test that alternates payloads for the same manifest ID.
Proposed direction
- previous_ids: set[str] | None = None + previous_snapshot: dict[str, dict[str, Any]] | None = None ... - document_ids = {str(document["id"]) for document in documents if document.get("id")} - observed_documents.update((str(document["id"]), document) for document in documents if document.get("id")) - if document_ids == previous_ids: + snapshot = { + str(document["id"]): dict(document) + for document in documents + if document.get("id") + } + observed_documents.update(snapshot) + if snapshot == previous_snapshot: matching_reads += 1 else: - previous_ids = document_ids + previous_snapshot = snapshot matching_reads = 1As per path instructions, knowledge-layer changes must cover external API consistency behavior with deterministic source-level tests.
🤖 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 `@sources/knowledge_layer/src/azure_ai_search/adapter.py` around lines 1168 - 1192, Update _stable_documents to compare normalized selected document contents, including fields such as status and chunk_count, rather than using only document ID sets to determine matching reads. Preserve deterministic ordering and existing stabilization thresholds, and add a deterministic source-level test that alternates payloads for the same manifest ID to verify changing contents are not treated as stable.Source: Path instructions
🤖 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.
Outside diff comments:
In `@sources/knowledge_layer/src/azure_ai_search/adapter.py`:
- Around line 1168-1192: Update _stable_documents to compare normalized selected
document contents, including fields such as status and chunk_count, rather than
using only document ID sets to determine matching reads. Preserve deterministic
ordering and existing stabilization thresholds, and add a deterministic
source-level test that alternates payloads for the same manifest ID to verify
changing contents are not treated as stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 16bb7ec5-51c4-46ba-9d5d-508054bea5f3
📒 Files selected for processing (2)
sources/knowledge_layer/src/azure_ai_search/adapter.pytests/knowledge_layer_tests/test_azure_ai_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/knowledge_layer_tests/test_azure_ai_search.pysources/knowledge_layer/src/azure_ai_search/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_azure_ai_search.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/azure_ai_search/adapter.py
🔇 Additional comments (2)
sources/knowledge_layer/src/azure_ai_search/adapter.py (1)
72-72: LGTM!Also applies to: 197-200, 625-673, 838-840, 882-922, 968-995, 1010-1038, 1070-1070, 1086-1123, 1194-1209, 1324-1402, 1452-1496
tests/knowledge_layer_tests/test_azure_ai_search.py (1)
76-109: LGTM!Also applies to: 122-122, 146-187, 290-293, 337-349, 668-668, 732-733, 745-1029, 1238-1258, 1287-1302, 1417-1417
|
/ok to test a4ebbaf |
|
/ok to test 9427276 |
|
/merge |
e9025bb
into
NVIDIA-AI-Blueprints:release/2.2
3 participants
Overview
Azure AI Search is eventually consistent. Upload and deletion results can therefore appear stale immediately after successful writes, causing premature job completion, incomplete cleanup, or inconsistent file state.
This PR:
The scope is limited to the Azure AI Search adapter and its tests. No API contract or configuration changed, so no documentation update is required.
Known bounded limitations remain: stable reads are a consistency heuristic; confirmation-timeout retry is guaranteed only within the current process and, after restart, depends on the file manifest still being searchable.
Validation
Strict live Azure lifecycle validation was repeated three times with fresh collections. All three runs passed, verifying immediate delete/list/query behavior, cross-collection isolation, deletion of the remaining file, and complete collection cleanup.
git commit -sor an equivalent sign-off.Where should reviewers start?
Start with
sources/knowledge_layer/src/azure_ai_search/adapter.py, especially the failed-upload rollback, stable visibility checks, anddelete_filebookkeeping order.Regression coverage is in
tests/knowledge_layer_tests/test_azure_ai_search.py.Related Issues
Summary by CodeRabbit