Skip to content

fix(memory): enforce authoritative revision integrity - #1303

Open
knqiufan wants to merge 2 commits into
oceanbase:masterfrom
knqiufan:fix/issue-1297-memory-integrity
Open

fix(memory): enforce authoritative revision integrity#1303
knqiufan wants to merge 2 commits into
oceanbase:masterfrom
knqiufan:fix/issue-1297-memory-integrity

Conversation

@knqiufan

@knqiufan knqiufan commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Which issue or RFC does this PR close?

Closes #1297.

Rationale for this change

MemoryUnitOfWork.commit() is the final persistence trust boundary, but it previously accepted incomplete or internally inconsistent revisions. A malformed direct SPI writer could persist illegal predecessor links or body/hash mismatches, and search could then return text that authoritative entry reads rejected.

What changes are included in this PR?

  • Validate the stored base, every history row reachable from its manifest heads, and the complete base-to-revision transition inside the commit transaction.
  • Require the manifest, changes, new entry versions, direct predecessors, and active projections to agree exactly.
  • Recompute entry hashes and validate declared/manifest hashes, canonical searchable text, and configured embedding bindings before any write.
  • Enforce scope-global entry_version_id identity with a database unique index, including startup migration and duplicate-data detection for existing SQLite and OceanBase schemas; translate insert races into MemoryBackendConfigurationError with full rollback.
  • Reject semantic revisions of inactive entries while retaining the narrow organize(normalize) path when canonical content bytes are unchanged.
  • Validate dense embeddings by dimension, finite values, and unit-norm tolerance without a second exact floating-point normalization comparison.
  • Revalidate entries(), expand(), projection reads, and every FTS/vector candidate against the exact authoritative revision, manifest, entry row, and current head projection.
  • Add regression coverage for malformed/cross-entry/cross-Memory/skipped predecessors, pre-existing corrupted history, missing/extra/duplicate rows, hash and projection corruption, inactive revisions, dense vectors, migrations, rollback behavior, and read/search/expand agreement.

Are there any user-facing changes?

Malformed commits made through the public Memory backend SPI now fail with MemoryBackendConfigurationError instead of becoming durable state. Existing SQLite and OceanBase schemas receive the scope-global entry-version unique index during startup; startup reports MemoryBackendConfigurationError if legacy duplicate identities must be repaired first. Valid service workflows and the documented no-vector degradation remain unchanged. There are no HTTP API changes.

How was this change tested?

  • uv lock --locked
  • uv run --no-sync prek run -a
  • uv run --no-sync ty check
  • uv run --no-sync python -m pytest tests/builtin/artifacts/memory tests/builtin/persistence/test_memory.py tests/builtin/persistence/test_memory_integrity.py tests/builtin/persistence/test_sqlite_profile.py tests/builtin/persistence/test_oceanbase_profile.py tests/e2e/test_memory_search_concurrency.py tests/e2e/test_builtin_runtime.py -q (60 passed, 7 skipped)
  • Full Windows run: 620 passed, 15 skipped; 8 unrelated pre-existing Windows failures remain in symlink privileges, the unavailable python3 command, POSIX mode bits, encoding, and path-separator assertions.
  • The live two-connection OceanBase race regression is opt-in through POWERCONTEXT_TEST_OCEANBASE_URL; it is skipped when no dedicated OceanBase test database is configured.

AI usage statement

OpenAI Codex (GPT-5) was used to inspect the issue and review feedback, implement the fixes, design regression tests, and review the final change.

@Teingi Teingi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening the Memory integrity boundary. The original issue is real and the main reproducer is addressed, but the three cases below still allow revisions outside the documented contract. Requesting changes until these integrity gaps are closed.

)
).mappings()
stored_rows = tuple(rows)
if any(str(row["entry_version_id"]) in new_by_id for row in stored_rows):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Make version-ID uniqueness database-enforced

Could we make this uniqueness check database-enforced? The SELECT and later INSERT are not atomic. On OceanBase, two transactions can both observe no row and insert the same entry_version_id for different memory_artifact_id values, because the current primary key and unique constraints include the Memory ID. Both commits can therefore succeed, leaving an ID that this code treats as scope-global duplicated. Please add a database-level UNIQUE (scope_id, entry_version_id) with an explicit migration for existing tables, translate the resulting integrity error, and cover it with a coordinated two-connection race where both reads complete before either insert.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this. I reproduced the two-transaction gap and confirmed that the SELECT/INSERT check was not sufficient. This is now fixed with a database-enforced unique index on (scope_id, entry_version_id), startup migrations for existing SQLite and OceanBase tables with duplicate-data detection, IntegrityError translation with rollback, and a coordinated opt-in OceanBase two-connection regression test.

canonical_base: Memory | None,
new_by_entry: Mapping[str, MemoryEntryVersion],
) -> None:
previous_ids = tuple(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Validate history already referenced by the base

This only collects predecessors for value.entry_versions, so an invalid version already referenced by canonical_base is never rechecked. I reproduced this by changing a persisted v1 row to previous_version_id="does-not-exist" and then appending an unrelated entry: the commit succeeds and the new Revision still references the broken history. Because older releases could already persist this state, please either validate the base-referenced chain here or provide a one-time audit/migration plus database constraints that make this assumption safe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed reproducer. I verified that an unrelated append could carry forward a broken predecessor already referenced by the canonical base. This is now fixed: the commit boundary validates every base-referenced chain back to v1, including identity, version sequence, predecessor links, revision ordering, and canonical hashes. A regression test covers corruption in a deeper ancestor.

and change.from_entry_version_id == previous.entry_version_id
and change.to_entry_version_id == current.entry_version_id
and current.entry_version_id != previous.entry_version_id
and current.state == previous.state

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reject semantic revisions of inactive entries

This accepts a revise whenever the state is unchanged, including inactive -> inactive. A direct SPI writer can therefore deactivate an entry containing “black tea” and commit an inactive revision containing “green tea”, although the Memory contract requires reactivation before a semantic revision. organize(normalize) still needs a narrow exception, so please allow an inactive revision only when the new and predecessor canonical content bytes are identical and the change is normalization; otherwise require reactivation first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. I reproduced an inactive semantic revision and confirmed that it violated RFC 0014. This is now fixed: inactive revisions are rejected unless they are the narrow organize(normalize) case and the predecessor/new canonical content bytes are identical. Both the rejection and the allowed normalization path are covered by regression tests.

except (TypeError, ValueError) as error:
raise _InvalidMemoryCommitError("vector") from error
if canonical != projection.embedding or expected_hash != projection.embedding_content_hash:
raise _InvalidMemoryCommitError("vector")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] 不要用精确浮点相等判断单位向量已经规范化

MemoryService._embed_texts() 已先执行一次 canonical_embedding();这里再次归一化后再做 tuple 精确比较。单位归一化不是位级幂等的:公开 MemoryService.remember() 探针让模型返回 (0.2407121489724894, -0.9705965492093231),服务层得到 (0.24071214897248938, -0.9705965492093231),这里二次归一化又把首项改成 0.24071214897248935,于是合法的有限非零向量被抛为 _InvalidMemoryCommitError("vector"),整个 Memory 写入失败。现有向量测试只使用 (1, 0, 0) 这类轴向量,无法覆盖该舍入路径。请改为校验维度、有限性和带容差的单位范数(或确保只规范化一次并复用结果),并补一个非轴向量的公开 MemoryService.remember() 回归测试。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感谢指正。我使用你给出的非轴向量复现并确认了这个问题:二次归一化后的精确浮点比较确实会误拒绝合法写入。现已修复为校验维度、有限性和带容差的单位范数,同时保留 profile/content hash 校验,并补充了公开 MemoryService.remember() 稠密向量回归测试及非单位向量拒绝测试。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Memory commit accepts malformed revisions and search can return text that entries() rejects

3 participants