Skip to content

PLT-454: LoadTx lifecycle timestamp plumbing - #41

Merged
bdchatham merged 3 commits into
mainfrom
brandon2/plt-454-loadtx-timestamps
Jun 11, 2026
Merged

PLT-454: LoadTx lifecycle timestamp plumbing#41
bdchatham merged 3 commits into
mainfrom
brandon2/plt-454-loadtx-timestamps

Conversation

@bdchatham

Copy link
Copy Markdown
Contributor

Implements PLT-454 — the foundation PR of the coordinated-omission fix for the sei-load workload modeler.

What

  • Add IntendedSendTs + reserved AttemptedSendTs / InclusionTs to LoadTx (types/scenario.go).
  • Stamp IntendedSendTs in Dispatcher.Run/RunBatch right before hand-off, where the dispatcher is the sole owner of the pointer (race-free, written exactly once).
  • Document the timestamp concurrency contract on the struct: single-writer-before-handoff, immutable after enqueue, InclusionTs owned solely by the future PLT-459 tracker.

Not in this PR (by design)

Nothing reads these fields for latency yet — the existing dequeue-relative path is untouched, so this is additive and behavior-preserving. PLT-458 moves IntendedSendTs to the true scheduled instant t₀ + i/λ; PLT-459 populates InclusionTs.

Tests

go build ./... clean; go test -race ./... all packages ok.

Decision brief: designs/sei-load-workload-modeler/PLT-454-loadtx-timestamps.md (sei-protocol/bdchatham-designs).

🤖 Generated with Claude Code

Add IntendedSendTs (+ reserved AttemptedSendTs, InclusionTs) to LoadTx and
stamp IntendedSendTs in the dispatcher before hand-off into the send pipeline.
Foundation of the coordinated-omission fix; pure plumbing, behavior-preserving
(the existing dequeue-relative latency path is untouched).

Documents the timestamp concurrency contract: single-writer-before-handoff,
immutable after enqueue, InclusionTs owned solely by the future PLT-459 tracker.

go build + go test -race ./... green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Jun 11, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Additive fields and one-time writes at existing ownership boundaries; no consumers or metric logic changes in this PR.

Overview
Adds lifecycle timestamp plumbing on LoadTx as the first step toward fixing coordinated omission in sei-load metrics.

LoadTx gains IntendedSendTime, AttemptedSendTime, and a reserved InclusionTime, plus documented single-writer / handoff concurrency rules (zero time = untracked). The dispatcher stamps IntendedSendTime immediately before enqueue in Run and RunBatch; workers stamp AttemptedSendTime at dequeue before the HTTP send. Nothing reads these fields yet—existing latency stats stay on the dequeue-relative path; IntendedSendTime is enqueue time today, not true scheduled t₀, until follow-up work.

Reviewed by Cursor Bugbot for commit 56eec2e. Bugbot is set up for automated code reviews on this repo. Configure here.

bdchatham and others added 2 commits June 11, 2026 14:43
Apply idiomatic + measurement review feedback:
- Rename IntendedSendTs/AttemptedSendTs/InclusionTs -> ...Time to match the
  package's existing time.Time naming convention (startTime, lastWindowTime).
- Drop ticket-number / rollout-state changelog prose from comments; describe
  each field's durable contract instead.
- Tighten the concurrency contract: timestamps are written by whichever
  goroutine owns the tx at that stage (ownership transfers via the channels),
  not "by the dispatcher before enqueue" — AttemptedSendTime is written by the
  owning worker post-dequeue.
- Add guardrail: IntendedSendTime currently holds the back-pressured enqueue
  time and must not be used to derive latency until an open-loop scheduler sets
  it; zero timestamp means untracked (prewarm / stage not reached), not t=0.

go build + go test -race (sender, generator, types) green; the flaky
probabilistic TestAccountPoolMixedRate (unseeded global RNG, addressed by the
seed work) passes 12/12 under -count=1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire the worker to stamp tx.AttemptedSendTime when it fires the send, removing
the dead reserved field and making its doc accurate. The worker solely owns tx
between dequeue (txChan) and the sentTxs hand-off, so the write is race-free per
the LoadTx concurrency contract. Additive — the existing latency path is
unchanged and nothing reads AttemptedSendTime yet (PLT-463's schedule_lag will).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bdchatham

Copy link
Copy Markdown
Contributor Author

Expert review — converged (no remaining concerns)

Reviewed by independent specialists (not the authoring agent), two rounds:

  • Idiom (Go): ✅ resolved — renamed …Ts…Time to match the package's existing time.Time convention (startTime, lastWindowTime); removed ticket-number/rollout-state changelog prose; concurrency-contract doc anchored to the real txChan/sentTxs channels.
  • Measurement semantics: ✅ resolved — IntendedSendTime now documents that it currently holds the back-pressured enqueue time and must not be used to derive latency until the open-loop scheduler sets it; ownership-transfer model corrected; zero=untracked documented. AttemptedSendTime is now stamped at the actual send attempt (was a dead field).

Deferred to PLT-458 (open-loop scheduler), recorded on that issue: reserve the per-tx sequence index i/epoch t₀ for the t₀ + i/λ schedule; account for dropped-tx zero-InclusionTime holes in any inclusion-rate denominator.

Behaviour-preserving (existing latency path untouched). go build + go test -race ./... green; the probabilistic TestAccountPoolMixedRate is a pre-existing unseeded-RNG flake (passes 12/12 under -count=1), addressed by PLT-456.

@bdchatham
bdchatham requested review from amir-deris and masih June 11, 2026 21:51
@bdchatham
bdchatham merged commit 8a63e19 into main Jun 11, 2026
3 checks passed
@bdchatham
bdchatham deleted the brandon2/plt-454-loadtx-timestamps branch June 11, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants