fix(memory): guard fetchone() against None in MemoryStore.add_fact and update_fact#22660
Open
wesleysimplicio wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes rare concurrent-delete edge cases in the holographic SQLite MemoryStore where .fetchone() can return None and previously caused TypeError, and adds targeted regression tests to cover those scenarios.
Changes:
- Add a
Noneguard inadd_fact()’s duplicate-handling path (afterIntegrityError) and raise a clearerRuntimeError. - Add a
Noneguard inupdate_fact()’s category re-fetch path to avoid subscriptingNone. - Add regression tests exercising both the normal dedupe path and the concurrent-delete edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
plugins/memory/holographic/store.py |
Adds fetchone()-None guards in add_fact() and update_fact() to prevent TypeError under concurrent deletions. |
tests/plugins/memory/test_holographic_store_null_guards.py |
New regression tests simulating fetchone() -> None after a duplicate insert and after an update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
173
to
+180
| except sqlite3.IntegrityError: | ||
| # Duplicate content — return existing id | ||
| # Duplicate content — return existing id. | ||
| # Guard against the (rare) cross-process concurrent-delete case | ||
| # where the row is gone by the time we SELECT it. | ||
| row = self._conn.execute( | ||
| "SELECT fact_id FROM facts WHERE content = ?", (content,) | ||
| ).fetchone() | ||
| if row is None: |
Comment on lines
+311
to
315
| _cat_row = self._conn.execute( | ||
| "SELECT category FROM facts WHERE fact_id = ?", (fact_id,) | ||
| ).fetchone() | ||
| cat = _cat_row["category"] if _cat_row is not None else "general" | ||
| self._rebuild_bank(cat) |
|
|
||
| @pytest.fixture | ||
| def store(tmp_path): | ||
| return MemoryStore(db_path=str(tmp_path / "test.db")) |
| """Regression tests for None-guard fixes in holographic MemoryStore. | ||
|
|
||
| Bug: add_fact() and update_fact() could crash with TypeError on fetchone() | ||
| when a concurrent process deletes the row between an UNIQUE violation and |
9118264 to
220739e
Compare
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
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.
What does this PR do?
Two methods in
plugins/memory/holographic/store.pycall.fetchone()["field"]without first checking iffetchone()returnedNone, causingTypeError: 'NoneType' object is not subscriptablein concurrent-delete edge cases.Root cause
Two methods in
plugins/memory/holographic/store.pycall.fetchone()["field"]without first checking iffetchone()returnedNone, causingTypeError: 'NoneType' object is not subscriptablein concurrent-delete edge cases.add_fact()(line 178)After a
sqlite3.IntegrityErroron a duplicateINSERT, the code does aSELECTto retrieve the existingfact_id. If a concurrent process deleted that row in the tiny window between the failedINSERTand theSELECT,fetchone()returnsNoneand the subscript crashes.update_fact()(line 299)After successfully updating a fact, the method re-queries
categorywhen the caller didn't supply one. Since this second…Fix
add_fact(): addif row is None: raise RuntimeError(...)before accessingrow["fact_id"].update_fact(): replacefetchone()["category"]with an explicit check, falling back to"general"when the row is gone.Why this shape
This shape mirrors #29640 so reviewers can quickly compare scope, root cause, fix, tests, and related context without having to decode a custom PR description.
Tests
Original body
Related PRs / issues
N/A
Original body
Summary
Two methods in
plugins/memory/holographic/store.pycall.fetchone()["field"]without first checking iffetchone()returnedNone, causingTypeError: 'NoneType' object is not subscriptablein concurrent-delete edge cases.What Changed
Fluxo
A mudança continua seguindo o fluxo original descrito na seção preservada abaixo, sem ampliar o escopo funcional deste PR.
Visão
A padronização melhora a revisão, reduz ruído e evita deriva de formatação entre PRs abertos.
Test Plan
Original body
What does this PR do?
Problem
Two methods in
plugins/memory/holographic/store.pycall.fetchone()["field"]without first checking iffetchone()returnedNone, causingTypeError: 'NoneType' object is not subscriptablein concurrent-delete edge cases.add_fact()(line 178)After a
sqlite3.IntegrityErroron a duplicateINSERT, the code does aSELECTto retrieve the existingfact_id. If a concurrent process deleted that row in the tiny window between the failedINSERTand theSELECT,fetchone()returnsNoneand the subscript crashes.update_fact()(line 299)After successfully updating a fact, the method re-queries
categorywhen the caller didn't supply one. Since this secondSELECTruns after theCOMMIT, a concurrent process could delete the row, causing the sameTypeError.Fix
add_fact(): addif row is None: raise RuntimeError(...)before accessingrow["fact_id"].update_fact(): replacefetchone()["category"]with an explicit check, falling back to"general"when the row is gone.Tests
tests/plugins/memory/test_holographic_store_null_guards.py(5 tests):test_add_fact_duplicate_returns_existing_id— normal dedup path still workstest_add_fact_concurrent_delete_raises_runtime_error— confirmsRuntimeError(notTypeError) on stale SELECTtest_update_fact_nonexistent_returns_false— non-existentfact_idreturns Falsetest_update_fact_with_category_skips_refetch— supplied category avoids the re-fetchtest_update_fact_concurrent_delete_uses_general_fallback— concurrent delete →"general"fallback, no crashStash-verified: both failing tests reproduce
TypeErrorat the exact lines (store.py:178andstore.py:299) without the fix.Checklist
pytest tests/plugins/memory/test_holographic_store_null_guards.py— 5 passedSolution Sketch
Related Issue
N/A
Type of Change
Changes Made
.github/PULL_REQUEST_TEMPLATE.mdHow to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Generated by Hermes Turbo
Generated by Hermes Turbo