Skip to content

fix(memory): skip non-text parts and use millis timestamps in memory search - #1468

Open
rootkiller6788 wants to merge 1 commit into
google:mainfrom
rootkiller6788:fix-memory-search-nontext-crash
Open

fix(memory): skip non-text parts and use millis timestamps in memory search#1468
rootkiller6788 wants to merge 1 commit into
google:mainfrom
rootkiller6788:fix-memory-search-nontext-crash

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

InMemoryMemoryService.searchMemory had two bugs:

  1. Crash on non-text parts. It called Part.text().get() unconditionally for every part in a stored event. When a session contains tool activity, the stored events include functionCall / functionResponse parts whose text() Optional is empty, so the next loadMemory search threw NoSuchElementException and the memory tool failed.

  2. Wrong timestamp unit. formatTimestamp used Instant.ofEpochSecond, but Event.timestamp() is epoch milliseconds (Event.build() defaults to Instant.now().toEpochMilli()), so returned memory timestamps were ~1000× in the future.

Changes

  • Skip non-text parts when building the keyword index (only text parts are searchable).
  • Format memory timestamps from epoch millis via Instant.ofEpochMilli.
  • Add InMemoryMemoryServiceTest covering both regressions.

Verification

  • New tests pass: InMemoryMemoryServiceTest (2 tests).
  • AgentWithMemoryTest still passes.
  • Full core module suite: 1776 tests, only pre-existing Windows path-separator failure in LocalSkillSourceTest.testListResources (fails on untouched HEAD too).

@sherryfox

Copy link
Copy Markdown
Contributor

Hi @rootkiller6788, thank you for your contribution and for taking the time to submit this PR.

You're right that InMemoryMemoryService.searchMemory has two bugs. The first fix unfortunately duplicates #1465, which should land soon. The second is definitely needed.

One ask for the scope then: once #1465 lands, could you rebase this down to just the formatTimestamp change? That drops the conflict and leaves a small, focused change that's easy to land — right now both PRs add the same new test file.

@sherryfox sherryfox self-assigned this Aug 28, 2026
@rootkiller6788

Copy link
Copy Markdown
Author

Thanks for the review, @sherryfox. I've narrowed the PR down to just the formatTimestamp fix as you suggested.

The timestamp test passes against the core module (1 test, no failures). Could you please take another look?

@sherryfox

Copy link
Copy Markdown
Contributor

Two mechanical things @rootkiller6788: check-commit-count is red at 2 commits, and the title still says "skip non-text parts" — worth retitling, since the title becomes the commit message and release-please turns it into the changelog entry.

@rootkiller6788
rootkiller6788 force-pushed the fix-memory-search-nontext-crash branch from 8ae0daa to 5269b87 Compare September 5, 2026 06:13
@rootkiller6788

Copy link
Copy Markdown
Author

Fix: memory search timestamps were parsed as seconds, not millis

InMemoryMemoryService.formatTimestamp() was decoding timestamps with Instant.ofEpochSecond(...). The timestamps going
through it are epoch millis, so the formatter read them as seconds and search result times came out wrong — anything
recent showed up as a date in 1970.

Changed it to Instant.ofEpochMilli(...), which is what the values actually are.

Tests

  • Added InMemoryMemoryServiceTimestampTest covering the timestamp formatting path.

This is the narrowed version of the earlier fix — just the timestamp bug, nothing else.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants