fix(managedstream): add stored timestamp cursor keys#266
Conversation
Greptile SummaryThis PR updates managed stream ledger exports to use stable timestamp cursor keys. It changes:
Confidence Score: 3/5This is close, but the legacy bad-timestamp path should be fixed before merging.
internal/guard/store/sqlite/ledger.go Important Files Changed
Reviews (2): Last reviewed commit: "fix(managedstream): add stored timestamp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa28484a7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
michiosw
force-pushed
the
feat/managedstream-stored-timestamp-cursor
branch
2 times, most recently
from
aa8281e to
17d7e2f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17d7e2f431
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| query := authorizationActionsSelectWithCursorKey() | ||
| args := []any{} | ||
| if opts.UpdatedAfter != nil { | ||
| updatedAfter := opts.UpdatedAfter.UTC().Format(time.RFC3339Nano) |
There was a problem hiding this comment.
When a caller provides UpdatedAfter without the new UpdatedAfterKey, this fallback still formats exact-second cursors as RFC3339Nano like 2026-06-08T12:20:07Z. The query now compares that text to fixed-width updated_at_cursor_key values such as 2026-06-08T12:20:07.000000000Z and .100000000Z; because . sorts before Z, updated_at_cursor_key > ? skips all later fractional rows in the same second. Managed stream supplies a key, but the store API still supports UpdatedAfter-only callers, so the fallback should use the fixed cursor-key layout as well.
Useful? React with 👍 / 👎.
|
@greptileai review |
| updatedAt, err := parseLedgerTimestamp(updatedAtKey) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
backfillAuthorizationActionCursorKeys now tolerates an unparseable legacy updated_at by storing the raw value as updated_at_cursor_key, but this path later parses that same key and returns the error. For a migrated row like the new act_bad_timestamp fixture, OpenStore succeeds, then LedgerBatch fails with invalid timestamp "not-a-timestamp" as soon as that row is selected for the cursor. Managed observe still stops before uploading any rows, so the lockout moves from migration time to export time.
michiosw
force-pushed
the
feat/managedstream-stored-timestamp-cursor
branch
from
17d7e2f to
6967a4e
Compare
1 participant
Where We Are
Katana managed observe can still fail hosted ingest when legacy ledger rows contain timestamps without a timezone, like 2026-06-08T12:20:07.853885. Hosted expects offset-aware timestamps, so those rows turn into 400 responses.
Where We Want To Go
Managed observe should upload the same ledger rows with valid UTC timestamps and keep cursor pagination stable. It must not skip rows, leak internal cursor fields into the payload, or rely on SQLite timestamp rounding.
How do we get there
This adds a stored Go-computed updated_at cursor key, backfills it during SQLite migration, uses it for bounded cursor filtering, and normalizes exported *_at fields to RFC3339 UTC. Covered by focused ledger/managed-stream tests and verified with go test -count=1 ./internal/guard/store/sqlite ./internal/managedstream plus go test ./...