fix: order AgentCore Memory events at millisecond resolution#572
Conversation
✅ No Breaking Changes DetectedNo public API breaking changes found in this PR. |
|
Claude Security Review: no high-confidence findings. (run) |
The monotonic timestamp counter incremented by 1 second on collision and stored state at class level. The 1s increment pushed a busy turn's events several seconds into the future (boto3/AgentCore Memory actually resolve eventTimestamp at millisecond granularity, not 1s), and class-level state was shared across managers for different sessions in the same process. Break ties at 1ms instead of 1s, and make the timestamp state instance-scoped.
da96548 to
b94474a
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| @@ -115,20 +113,12 @@ def _get_monotonic_timestamp(cls, desired_timestamp: Optional[datetime] = None) | |||
| if desired_timestamp is None: | |||
| desired_timestamp = datetime.now(timezone.utc) | |||
There was a problem hiding this comment.
We should floor the desired_timestamp, so two events within the same millisecond count as a tie. Timestamp as set here will be in microseconds, and comparison on line 117 will evaluate as a tie if within the same millisecond, but not the same microsecond.
There was a problem hiding this comment.
Good catch, you're right. datetime.now() gives microseconds but the service only keeps milliseconds, so two events in the same ms but different microseconds would pass the tie check and then collide once stored.
Fixed it by flooring desired_timestamp to ms before the comparison. Now same-ms events are treated as a tie and bumped 1ms apart. Added a regression test for the same-ms/different-microsecond case.
Fixes #571.
Problem
The monotonic timestamp counter added in #83 increments by 1 second on collision and stores its state at class level. Two consequences:
Events get pushed seconds into the future. Every same-second collision bumps the timestamp by a full second, so a turn that writes several events in quick succession stamps them 1s, 2s, 3s… ahead of wall-clock. The premise behind the 1-second step ("boto3 doesn't support sub-second resolution") does not hold: the
bedrock-agentcoreclient serializeseventTimestampas a unix timestamp with millisecond precision, and the service preserves it (theeventIdprefix is a millisecond epoch).State is shared across unrelated sessions. Because
_last_timestampand the lock are class attributes, concurrent session managers for different sessions in the same process perturb each other's ordering.This future-dating is what makes events reorder when a session is written by more than one process (e.g. consecutive turns served by different pods): a turn's events end up several seconds ahead, and a later turn written from a fresh process at real wall-clock time can sort before them, interleaving the two turns. Interleaving can place a
toolResultbefore itstoolUse, which the Converse API rejects.Changes
eventTimestampat. A colliding timestamp advances by 1ms rather than inflating by a full second, so a multi-event turn stays within milliseconds of real time instead of drifting seconds ahead._last_timestamp) so concurrent managers for different sessions no longer share or perturb each other's ordering.Behavior is otherwise unchanged: the first timestamp and any timestamp later than the current floor still pass through untouched;
Nonestill falls back todatetime.now(timezone.utc).Scope
This keeps ordering correct within a process and removes the multi-second future-drift that caused cross-process interleaving in practice. It does not add explicit cross-process coordination — a freshly started process still begins from wall-clock time — but because events are no longer pushed seconds into the future, sequential turns handed off between processes with any real gap no longer collide. Guaranteeing ordering under genuinely concurrent writers to the same session would require server-side sequencing and is out of scope here.
Testing
Unit tests —
TestMonotonicTimestamp(6 tests): instance-scoping (state not shared across managers), 1ms-not-1s tie-break, passthrough of the first / a later / aNonetimestamp, and a same-second burst that stays strictly ordered at millisecond spacing. Full memory-integration module: 178 passed locally (2 unrelated async tests require apytest-asyncioplugin not installed locally; unaffected by this change and green in CI).Multi-process experiment against real AgentCore Memory (subprocess-per-"pod"): with the old +1s counter, a 5-event turn drifted ~4s into the future and a follow-up turn from a fresh process interleaved deterministically (0/10 ordered). With the 1ms tie-break the same scenario stays ordered (10/10). A real Strands agent + real Bedrock model run confirmed the counter drifts a turn ~1.5–3.7s into the future on
main(each collision = +1s), which this change eliminates.