fix(ingest): resolve advert endpoints after node updates - #166
Conversation
|
@MrAlders0n, please review this focused fix (and have Claude review it if useful). It is independent of the analytics-retention work and can merge separately. Native Pi/PostgreSQL validation passed for 74f16de. The formal review-request API is not available to this contributor account, so I am requesting review here. |
74f16de to
2c5ad5f
Compare
|
@MrAlders0n, ready for your / Claude's review at The preview/source now runs server |
2c5ad5f to
e1c4fa2
Compare
|
@MrAlders0n / Claude: #166 is refreshed onto The fix now keeps your optional new-path repeat events: inserted observations apply their payload side effects before live endpoint resolution; repeat events resolve the current name without applying an older advert again. Suppressed copies do neither live endpoint lookup nor repeated side effects. First/renamed-advert regressions failed on the upstream ordering and pass here; the added repeat regression checks the latest name and lookup/upsert counts. Focused diff. Actual-head CI passes. The Pi composition also passes full PostgreSQL tests and a 504-scope replay of 3,200 inputs during blocked route maintenance, with all expected rows/events and zero fixture drops. All pending features are still in the preview; no upstream merges were performed. |
MrAlders0n
left a comment
There was a problem hiding this comment.
The fix is right, and the bug is still on dev. The lookup on dev still runs before handlePayloadTypeSideEffects. This needs a rebase onto dev (conflict in internal/ingest/packet.go with #178 and #179):
- Keep #178's
repeatlogic:if inserted || repeat { if inserted { side effects + MarkSent } ; lookup ; event }. - Drop the "Duplicates emit no packet event" comment. It's no longer true with
includeRepeats. - Drop the
InsertObservationoverride inendpoint_matching_test.go. #179 already setsobservationInserted. - Optional:
UpsertNodealready returns the node id, so the advert path could skipGetNodeByPubkey. - An advert repeat test (
includeRepeatson, duplicate on a new path) would be a nice addition.
|
@MrAlders0n / Claude: addressed at The #178/#179 integration is retained: inserted observations run side effects/MarkSent before lookup and event; repeats do not reapply old adverts. The outdated duplicate-event comment was already removed, and the redundant InsertObservation override is now gone. The advert-repeat regression passes. I left the optional UpsertNode-ID shortcut separate to avoid changing side-effect return plumbing during this correction. Published-head CI and the complete combined native Pi/PostgreSQL suite pass. The Pi runs server Please take another look when convenient; no upstream merges were performed. |
First or renamed adverts could broadcast missing or old source names because live resolution ran before the node update. Apply payload side effects before resolving inserted observations; optional new-path repeats resolve current identity without reapplying the old advert. Suppressed copies do no live-only endpoint work.
Closes #164. Independent on accepted dev.
Parent
db30c9b573357990c41166292e7cf42785c92cc4, head319df7da5246a71f9693f923d2124bf831eabb82. Focused diff.Published-head CI and the complete combined native Pi/PostgreSQL suite pass. The Pi runs server
a35cba1d/ webe1133ab5, retaining every review candidate. Windows/Pi web build/lint and all 940 tests pass. The isolated 3,200-input/504-scope replay preserved expected data/events with zero fixture drops; this is not a universal production losslessness claim. Validation, exact heads and recovery / running source.Maintainers retain merge/release control; AI-assisted work continues under the contributor's standing authorization.