feat(dotc1z): surface discovered_at through reader#1025
Conversation
c1z entries store a per-entry discovered_at (the time the connector actually observed the datum), but it is stripped when storage records convert to c1.connector.v2 protos: V3GrantToV2 never copies r.GetDiscoveredAt(), and v2.Grant has no field to carry it. c1 needs it to compare optimistic overlay grants against the real observation time instead of a coarse per-sync timestamp. Add GrantDiscoveredAtReader, an additive optional Reader capability in the exact mold of EntitlementGrantDigestReader / ExpansionGrantLister: type-asserted, Pebble-implemented, addressed by the same grant id GetGrant accepts. It returns the STORED discovered_at verbatim (never a fresh now()) and leaves v2.Grant untouched, so the connector API contract is unchanged. The deprecated SQLite engine does not implement it; a failed type assertion is the caller's "unavailable, fall back" signal. Round-trip tests cover both engines: Pebble returns the stored value (exact seeded timestamp, stable across reads), SQLite cleanly reports unavailable. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
General PR Review: feat(dotc1z): surface discovered_at through readerBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This is a fully additive change: a new standalone Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // GetGrantDiscoveredAt implements connectorstore.GrantDiscoveredAtReader. | ||
| // It returns the discovered_at stored on the grant addressed by grantID | ||
| // — the value stamped when the grant was first written and preserved | ||
| // across re-syncs and expander rewrites, NEVER a fresh now(). It reads |
There was a problem hiding this comment.
This comment is incorrect. Discovered_at is basically the time at which we crawled the row. If we start a new sync, the same resource will have a new discovered_at.
| return nil, false, nil | ||
| } | ||
| return rec.GetDiscoveredAt(), true, nil | ||
| } |
There was a problem hiding this comment.
Is uplift going to get the grant, then call GetGrantDiscoveredAt for that grant? If so, that would double the number of grant reads for a given uplift.
There was a problem hiding this comment.
If we start a new sync, the same resource will have a new discovered_at.
Right. Since the problem right now is flipping overlays, part of me is thinking if we should do this only for overlays.
|
This would be fixed if we exposed a v3 set of APIs? We should just do that IMO with all of them. It would be cleaner and sound. |
3 participants
What & why
c1z entries store a per-entry
discovered_at— the time the connector actually observed a grant. It's persisted in the storage layer but stripped when storage records convert toc1.connector.v2protos:pebble.V3GrantToV2never copiesr.GetDiscoveredAt(), andv2.Granthas no field to carry it. So c1 can't read it.c1 needs it: its uplift deletes optimistic "overlay" grant records by comparing
overlay.created_atagainst a single coarse per-sync timestamp, which can delete an overlay before the connector has actually re-observed the grant (→ a user loses access). The real fix compares against the per-entrydiscovered_at. This PR stops dropping it — it surfaces it through the reader.Mechanism chosen: A — additive optional reader capability
New interface
connectorstore.GrantDiscoveredAtReader, in the exact mold of the existingEntitlementGrantDigestReader/ExpansionGrantLister:grantIDGetGrantaccepts (stored external id, or the legacy reconstructed concat). The Pebble impl reusesGetGrantRecordand returnsrec.GetDiscoveredAt().v2.Grantis untouched → thec1.connector.v2API contract is unchanged, keeping the review surface narrow.Why A over B (adding a field to
v2.Grant): B is simpler for c1 to consume but mutates the connector API contract (proto field on a wire type shared by every connector) — a much wider blast radius for a value only c1's uplift needs. A is fully additive and mirrors how every other engine-specific read capability in this package is already exposed.Semantics
discovered_at, stamped once at first write and carried forward across re-syncs / expander rewrites — never a freshtime.Now().found=false(nil error) = "no such grant"; an ambiguous concat id stays an error (never a guess), matchingGetGrant.found=truewith a nil timestamp = a grant stored without a stamp (legacy) → caller treats as "unknown", not epoch zero.Scope
discovered_atin storage — a parallelGetEntitlementDiscoveredAtis a cheap follow-up if the "absence/coverage" signal ever needs it, deliberately left out here to keep the surface minimal.EntitlementGrantDigestReader.Files touched
pkg/connectorstore/connectorstore.go— newGrantDiscoveredAtReaderinterface + doc.pkg/dotc1z/engine/pebble/adapter_reader.go— PebbleGetGrantDiscoveredAtimpl.pkg/dotc1z/engine/pebble/adapter.go— compile-time interface assertion.pkg/dotc1z/engine/pebble/grant_discovered_at_reader_test.go— exact-seed round-trip, unknown-id, nil-stamp.pkg/dotc1z/grant_discovered_at_reader_test.go— cross-engine: Pebble returns stored value (stable across reads); SQLite cleanly reports unavailable.Tests
All new tests pass; full
pkg/dotc1z+ Pebble suites green. Changed files are lint-clean.🤖 Generated with Claude Code