fix(botevent): monotonic bot event id allocator behind an activation gate (#697)

[an9xyz]

Summary

Bot event ids — the sorted-set scores of robotEvent:{robotID} — came from
octo-lib GenSeq, a per-process HiLo block allocator. The queue is read with
an exclusive cursor (ZRANGEBYSCORE key (cursor +inf) and acked by score
(ZRemRangeByScore(id, id)), both of which require strictly monotonic, unique
scores. With three replicas they are neither, and events are permanently lost.

This PR replaces the allocator with a per-bot Redis INCR counter behind an
activation gate whose state is authoritative in MySQL. Merging keeps the same
allocator: until an operator activates, every replica delegates to GenSeq.

The one thing that is not the same on merge, stated here rather than further down
because it is the merge-safety claim (review rounds 5–7 all raised it): a
pre-activation allocation now costs one Redis round trip before delegating —
probeAllocatorState, which reads the mode mirror and whether this bot's counter
exists. GenSeq served 999 of every 1000 allocations from a process-local block with
no I/O at all. There is no DB round trip (pinned by
TestNotActivatedDoesNotQueryTheAuthorityPerAllocation), and for queue writers there
is no new dependency either — the ZADD on the same path already requires Redis.
addInlineQuery is the sharpest case and worth naming: it writes an in-memory map and
previously needed no Redis at all to obtain an id.

Scope, stated up front: this removes the collision and cross-restart
inversion classes of loss. It does not make the exclusive cursor lossless.
Allocation and publication are two operations, so a producer that allocates N and
stalls while another publishes N+1 still loses N once the consumer's cursor passes
it — and the doorbell makes that more likely, since the wake is triggered by the very
ZADD that creates the inversion. That residual is unchanged here, is now pinned by a
test asserting it still happens (TestKnownResidualZaddReorderingCanStillSkip),
and needs a re-delivery window below the cursor, which
modules/bot_api/events.go:186-199 itself names as the other half of the fix. An
earlier revision of this description called the result lossless; that was wrong.

Related Issue

Part of #697 — this PR does not close the issue. It is behaviour-neutral on merge
(the allocator stays on the legacy path until an operator activates it), so closing
requires deploy + activation + the three checks in that issue's acceptance section.
Do not use a closing keyword when merging. Related to #698, #700, and
Mininglamp-OSS/octo-lib#114 for the upstream half of the root cause.

Merging is not activation clearance. #704 carries two rollback-detection findings
from the fourth review round that are deliberately not fixed here, and it must be closed
before botevent-seq -action activate is run anywhere. That ordering is safe because
this revision makes the merged state genuinely inert (see Round 4 below); it would not
have been safe against the previous head, where a stray Redis key could activate the
counter on its own. The tool's -yes refusal names #704 so the gate is not only in this
description.

#697's acceptance criteria were narrowed to match what this PR actually delivers —
collisions and block-driven inversion — with the allocate-then-publish reordering
window split into a follow-up. The rationale is recorded on that issue; the short
version is that the two failures differ in kind (systemic and self-worsening versus
single-event and non-accumulating), and closing the reordering window first requires
resolving an atomicity-boundary conflict with modules/bot_mention, which commits an
idempotency claim atomically with the queue write and therefore needs the event id
before publishing — the opposite of assigning it inside the publish. I prototyped
the atomic version far enough to confirm that conflict, then reverted it rather than
grow this PR into a cross-module design change.

Linked Spec

.octospec/tasks/bot-event-score-monotonic/brief.md (included, with a
"实现记录" section recording where implementation and review corrected the
original brief).

The defect, measured

One bot queue, 644 events: 19 ordering inversions, every one on a 1000-block
boundary, max time regression 6.7 days. Across 1948 queues, 2624 colliding
scores in three of them, still accumulating (one queue's duplicate count
went 170 → 358 within an hour, five hours after the last restart — so the
addOrUpdateSeq write-back races on the extend path too, not just on cold
start). Every duplicated score holds exactly two members and every duplicate
range starts at …X001, i.e. min_seq + 1.

tools/genseq-repro reproduces all three loss paths against the real
config.Context.GenSeq and the real read/ack shapes:

1. Collision is deterministic, not a lucky race. Two child processes whose
   cold-start reads both precede either write-back issue the same 12 ids.
2. A page boundary landing on a shared score — the next read's exclusive
   (cursor excludes the second member forever. No time inversion needed;
   getEvents uses limit 20..100, so on a queue with >1000 duplicates this is
   routine.
3. A late enqueue from a lower block — those events are born below the
   cursor and no amount of polling reaches them.
4. Ack collateral damage — ZRemRangeByScore(id, id) deletes every member
   sharing the score, so acking a delivered event destroys an undelivered one.

Field reports match the mechanism's staging exactly: a card works at first, then
some users in a group cannot act on it, then no newly sent card is interactive at
all. The cursor is monotonic and never rewinds, so once it enters a high block
every id the main writer issues from a lower block is invisible — and each
restart opens yet another block, so it recurs.

Changes

Allocator (pkg/botevent/seq.go)

• Per-bot Redis INCR, in the same instance and db as the queue. That
  co-location is the durability argument, not an accident: production runs
  appendonly no, so a crash rewinds the counter — but it rewinds the queue with
  it, so resuming cannot collide with anything that survived. Recorded as a hard
  constraint, because "moving the counter somewhere more durable" would silently
  break it. maxmemory 0 / noeviction rules out eviction.
• First use per bot seeds above max(queue ceiling, legacy seq row) + 2×step.
  Both sources are needed: the queue covers ids already enqueued, the seq row
  covers a bot whose queue has drained but whose clients still hold a high cursor.
• Allocation failure fails the enqueue. No fallback — two live id sources on one
  queue is the defect.

Activation state is authoritative in MySQL

octo_bot_event_seq_state (singleton: mode / epoch / cutover_floor), flipped
by a FOR UPDATE compare-and-set that validates the floor against the observed
maximum and refuses with ErrFloorTooLow otherwise. Same shape as
octo_message_extra_version_state.

Review found that keeping the mode only in Redis was unsafe: production runs
appendonly no, so an RDB rollback drops it, and falling back to legacy then issues
GenSeq ids below everything the counter had handed out — #697 mirrored. The Redis
key is now only a mirror, whose sole purpose is letting the hot path check the
mode inside the same Lua script as the INCR. A missing mirror makes the allocator
read the DB row and, if activated, rebuild the mirror and re-seed rather than degrade.

FOR SHARE drain barrier is deliberately not copied from #627: a robotEvent
writer is INCR + ZADD with no transaction, so there is nothing to hold a lock
until, and wrapping each allocation in a transaction to borrow the barrier would
reintroduce the per-message DB round trip the allocator exists to avoid. The
consequence is documented rather than hidden: this design cannot drain in-flight
writers at the flip, and relies on the operator confirming no pre-fix replica runs,
plus a brief write pause.

Activation gate — one entry point, asymmetric cache (pkg/botevent/mode.go)

Round 4 found the gate had ordering and trust gaps of its own, all three of them the
same missing thing: no single stateful entry point for "is the counter activated". The
invariant is now one line — the authority decides, the mirror can only shortcut,
never override — and the cache behind it is deliberately asymmetric:

• A positive belief (activated) is terminal with respect to legacy. Once a process
  resolves incr it may refresh the epoch upward but can never decide legacy again —
  not on a DB error, a rolled-back row, or a dropped table. The downgrade branch is
  gone, not guarded.
• A negative belief is trusted only while the mirror agrees with it. A mirror
  claiming incr against a cached legacy is a conflict, and a conflict forces a
  fresh authority read. This is what preserves the no-divergence-window property the
  previous revision achieved by reading the DB per allocation: propagation of the flip
  never waits for a TTL, because the first allocation that sees the new mirror re-reads
  the authority. The TTL only bounds the case where the operator's mirror write failed.
• A confirmed conflict is a forged mirror: legacy, loudly
  (dmwork_bot_event_seq_mirror_unauthorized_total). It does not split the fleet,
  because every replica reads the same row and reaches the same conclusion —
  consistency comes from the authority, not from the mirror. That sentence is what the
  previous revision was missing.

The gate still reads the uncached mirror inside the same Lua script as the INCR,
so the brief's D2 reasoning is preserved, not overturned: what D2 forbids is deciding
the gate from a cache, and only the authority behind the mirror is cached.

The mirror value carries the generation (incr:{epoch}) and the gate compares against
the exact string this process validated, so a hand-written SET botEventSeq:mode incr
cannot open the gate even for one allocation — it carries no generation, so all it can
do is force an authority read. This is the mechanism epoch's column comment claimed
and did not have.

Ordering: seed, then publish the mirror. Writing it first opens the gate for every
bot in every replica while only one has been raised to its floor. And a lost mirror now
invalidates every seeded marker, not just the bot being allocated for — a lost mode
key is evidence Redis lost data, so every counter this process believes it seeded is
suspect. The mirror is global; a seed is per-bot.

tools/botevent-seq (preflight / activate) cannot verify the three
preconditions that matter, so it demands -yes and now names all three: #704 closed,
no pre-fix replica remaining, and the brief write pause. No online deactivate: going
back would put legacy ids below consumer cursors, the same loss mirrored.

Producers

• All queue writers route through botevent.NextEventID, enforced by a source guard.
• Queue keys are fully consolidated onto botevent.QueueKey now. Two earlier
  revisions of this description got this wrong in opposite directions — first claiming
  it was done, then correctly reporting four of five. rb.robotEventPrefix and
  modules/bot_api's own constant are both deleted, so there is one spelling and the
  chokepoint guard can see all of it.
• payload.robot_id is validated against existRobot like the three sibling branches
  beside it. It is the only branch whose value comes verbatim from a client's message
  payload, and the allocator turns an unknown value into a permanent Redis counter
  key (no TTL, noeviction) plus a seq row that is never reclaimed — where GenSeq
  cost only a TTL'd queue key and a row. This closes one of the three items review asked
  a human to verify; it was answerable from the code.
• addInlineQuery shares the same source. Its events are merged with the queue
  by getEventsResult, sorted by EventID and filtered by one shared cursor, so
  a separate sequence would push the cursor into the millions and permanently
  filter every ordinary event behind it.

Guards

Two source guards, both proven able to fail: every queue ZADD must go through
the allocator, and no bot event id may come from GenSeq outside the single
allowlisted delegation. The second guard additionally fails if that delegation
disappears — deleting it as dead code would make a deploy switch allocators on
its own. A docstring claiming a single chokepoint was already wrong once
(PR #685); these are tests instead.

Round 7 findings fixed here

One blocking finding, and the reviewer reproduced it against real MySQL + Redis. I reproduced
it too — the test was written and watched failing before any code changed, which is the
discipline the previous three rounds were missing.

finding	resolution
yujiawei P1-1 — the mirror repair added last head is a fleet-wide outage in the case it was meant to help. allocate has counterExists at seq.go:596, deletes the mirror at :604, and only at :611 uses that same value to conclude "the authority regressed". So it destroys the mirror on the authority's word before establishing the authority is the unreliable party. One cold replica then takes every healthy activated replica's gate down	The allocator never writes the mirror on the legacy path now. Not the suggested !counterExists guard — that narrows the window without closing it (the mirror is global, a counter is per-bot). The delete's only benefit was suppressing the read storm, which the cooldown also does, so it is gone rather than conditioned
P2-1 — activate is not idempotent and its refusal asserts a fact it never checked: it fatalled on any mirror claim without reading the state row, so re-running after a successful flip died telling the operator to DEL the live mirror	Reads the authority first. Three cases separate cleanly. Plus the tool's first test file — it carries the whole activation procedure, is the only thing a human runs at the cutover, and had produced two findings across two rounds with zero tests
Deviation 1 — "behaviour-neutral … exactly as before", raised in three consecutive rounds	Fixed in the claim, not further down: the added Redis round trip is now stated in Summary beside the sentence it qualifies, with addInlineQuery named as the only genuinely new dependency
Deviation 2 — mode.go's "the race is benign" was wrong in both halves	Gone with the mechanism it described
P2-3 … P2-6	plausibleRobotID says it bounds bytes deliberately (and why utf8mb4 characters differ, and why that is unreachable today); the write-only confirmed field is deleted; install's never-returned error is gone; invalidateSeeded's docstring states the real, smaller stake
P2-2	The retained per-bot counter comment now says why bot-count-scale leakage is accepted where payload-scale was not — the unit of abuse differs
P2-7 / P2-8	#704, which gained Gap 5: mirror repair has no safe automatic form (both attempts and why each failed), plus the two things it leans on that nobody has audited

The cost of the fix, stated rather than hidden. The denial cooldown is back — 1s, keyed on
the exact denied value — so a genuine activation writing byte-identical bytes to a previously
denied mirror is not noticed for up to that second. That window cannot be zero without reading
the authority per allocation, because nothing visible from Redis distinguishes a forged
incr:1 from the real one. It is only entered when a mirror claiming activation already sits
against a legacy authority, which botevent-seq now refuses to flip on top of, and it is
armed only when no counter exists — with one, the authority is the regressed party, every
allocation for that bot is refused anyway, and suppressing the read would only delay noticing
recovery.

Round 6 findings, and one decision that needs naming

Both blocking reviewers found the same thing again: the fix from the previous head was
wrong in a new way. So one item here is a decision rather than a patch.

finding	resolution
yujiawei P1-1 — the span bound is over its bound on the first allocation after every seed, deterministically, for every bot in every process. First failed durable write refuses with no grace and drops the event; the recovery probe becomes a storm inside msgSem slots (~67 producing bots saturate 100)	Moved to #704 in one piece, with the arithmetic — see below
Jerry-Xin 🔴 / yujiawei P2-1 — the denied-mirror cache swallows the genuine activation: Activate bumps epoch 0 → 1 and the tool writes exactly incr:1, byte-identical to a forged incr:1 already in Redis	The contradicted mirror is removed (compare-and-delete on the exact value) instead of remembered. No cache, nothing to go stale. botevent-seq also refuses to flip while such a key is present
yujiawei P1-2 — invalidateSeeded's delete order can leave a bot marked seeded with its companion state wiped	seeded cleared last; the worst interleaving now leaves a bot unseeded, which the next allocation fixes
yujiawei P2-3 — the staleness shortcut can answer from a belief resolved against a different mirror observation	Re-checks the mirror claim before serving a negative belief
yujiawei P2-2 — four documentation deviations, including the one string an operator reads during an incident (EXPECTED_MODE=incr will not unblock a counterExists refusal — it is a fail-closed assertion)	All corrected, including Rollout step 6, which now names three metrics and says which one does not self-heal
yujiawei P2-5 — the existRobot fail-open path can adopt an arbitrary client string	Syntactic bound first: length ≤ the robot.robot_id column width, no whitespace or control characters. No charset allowlist — the column stores whatever it is given
spec ❌ — the score-source guard was neither built nor moved	Recorded as #704 Gap 4 and marked DEFERRED in the brief

Why the durable-mark bound leaves this PR

I worked the arithmetic out before patching it a third time, and it says a patch cannot fix
it:

A seed sets the counter to  S = max(sources) + seedSafetyMargin
A recovery seed replays the same computation over the surviving MySQL sources, so it
lands at S again. Ids issued after a seed are S+1, S+2, … — all above S.

So the first id after any seed is already outside what a recovery from an unchanged mark
could reach: the exposure starts immediately, not after some grace. Closing it requires
advancing the mark at seed time, and a mark recording the seeded value feeds back into
the next seed's floor — every re-seed compounds by a block, which also breaks the no-op
property TestSeedIsIdempotentAndNeverLowers guards. That is a change to what the
recovery floor is, which is the same question #704 already owns for rollback detection.

Both reviewers recommended exactly this, and yujiawei flagged it as needing an owner's call.
Taken: persistHighWater keeps the throttle and the metric, and now says plainly that
the mark trails without bound while writes fail.
TestFailedDurableWriteIsThrottledAndDoesNotFailTheEnqueue asserts the gap is still
there, so it becomes the regression test the day #704 closes it. #704 gained the
arithmetic and both failed attempts so the road is not taken a third time.

This is strictly more honest than either previous revision: one asserted a bound it did not
have, the other over-triggered and dropped events. Neither delivered the property, and the
exposure is unreachable before activation — which #704 gates.

Round 5 findings fixed here

Both blocking reviewers found the same defect, and it was mine from the previous head:
the P1-7 bound recorded as fixed did not fail closed. Six items fixed here; the two
rollback-detection findings stay on #704.

finding	what it was	fix
Jerry-Xin 🔴 / P1-B	The bound counted failed intervals, but the throttle short-circuited before consulting it and re-armed unconditionally — so past the bound only 1 allocation in 1000 failed and 999 succeeded against a frozen mark. Arithmetic put ids at M+2999 against a margin of 2000, so a restarted process silently resumed at M+2001 beneath cursors already at M+2999	Bound is now the span past the last mark that landed, refused at seedSafetyMargin, checked on every allocation. The failure counter (and its non-atomic read-modify-write) is gone
P1-A	mode.go's central invariant statement was false — a positive belief never re-reads the authority, so a post-activation authority rollback does split the fleet — and #704's scoping argument was quoting it	Statement corrected; the same round trip that reads the mirror now also reports whether the bot's counter exists, which is cross-process proof of activation, so a cold process refuses instead of degrading. No DB read, no env var
P1-C	A denied mirror cost an authority read per allocation, serialized behind one mutex inside a msgSem slot, with no exit — nothing clears a forged key. Same hazard class as the P1-1 read removed last head	Denial cached keyed on the exact mirror value denied; forced refreshes serve waiters from a read someone else already did
spec deviation	"Queue keys are fully consolidated" — third revision to state this wrongly. Four Del sites still formatted the key, plus a dead field	Finished. grep -rn '"robotEvent:' modules/ is empty outside tests
spec deviation	payload.robot_id dropped the event when existRobot errored, turning a DB blip into silently lost bot events	Falls through to the old behaviour on error. The check now only rejects when the bot is known not to exist
P2	Doorbell guard matched "robotEvent:%s" but not "robotEvent:" + robotID; TestMain probed SELECT 1/PING but not the two CREATE TABLEs that actually gate the tests; the expected-mode test hook raced with production reads	All three closed
P1-D	payload.robot_id is checked for existence, not authorization (pre-existing)	Routed to a human owner as the reviewer asked; not changed here

Two things the tests forced out, worth naming because a read-only review cannot see
them:

• The span bound could not self-heal. With the ordinary id-distance throttle,
  recovery waited for ~1000 more refused allocations after the DB came back — unbounded
  wall-clock time for a low-traffic bot. Past the bound the probe now uses a time budget.
  It has to stay throttled: a failing INSERT with a 300ms deadline in a held msgSem
  slot is a throughput cliff for every bot in the process, doomed enqueue or not.
• The span cannot be checked before the write attempt. A bot with no durable row has
  a base of 0, so checking first refuses its very first allocation before trying to
  record it.

Round 4 findings fixed here

Both blocking reviewers independently found the same critical defect, and one found a
regression the previous head introduced. Six of the eight P1s are fixed here; two are
#704.

finding	what it was	fix
P1-2 / 🔴	A positive mirror was trusted on the hot path and the authority was never read, so a stray key performed the gated activation on every replica	Authority consulted once per process before a positive mirror is believed; mirror carries the generation
P1-1	Pre-activation did an un-deadlined SELECT per allocation inside a held msgSem slot — the merge was not behaviour-neutral, and it re-entered the hazard aedde27a bounds	Asymmetric belief cache; one read per process per interval. Pinned by a test that asserts the I/O shape, since the existing test asserts only the id's shape
P1-4	mirrorMissing could return a GenSeq id for a bot this process had already issued counter ids for	Positive belief is terminal; ReadState distinguishes missing-table from unreachable; legacyDelegate refuses if lastIssued holds the bot; malformed EXPECTED_MODE fails closed
P1-3	The global mirror was published before the per-bot seed	Seed first; a lost mirror invalidates every seeded marker
P1-7	A failing durable write neither throttled (one INSERT per event) nor bounded the exposure the seed margin is supposed to cover	Interval guard re-armed on failure; bounded at highWaterFailureLimit consecutive intervals, then allocations fail
P1-8	Activation evidence came only from SCAN robotEvent:*, so a drained-queue bot contributed nothing to the floor that protects it — and scalarSeq turned every DB error into 0	Table-wide MAX(min_seq) per namespace; refuses partially failed evidence as it already refused sampled evidence
P1-5 / P1-6	Rollback detection is tolerance-bounded and process-local, and the "next seed will fix it" rationale is contradicted by the code (seeded.Delete only appears in already-detected branches)	#704, gated on activation not merge. One design question, only reachable post-activation — which this revision is what makes true

DB deadlines no longer depend on the DSN: every call on this path uses
LoadContext/ExecContext with an explicit timeout, so the "does production set
readTimeout?" question stops being this path's only protection.

P2s fixed: the migration Down refuses while mode=1 (P2-2); the floor is rejected
above 2^50, where float64 scores stop distinguishing int64 ids and the tool would
recreate the collisions it exists to prevent (P2-3); a TestMain fails loudly under
CI=true instead of letting the whole integration suite silently reduce to source
guards (P2-11 — the same failure mode as the botevent_test database an earlier
revision used); the GenSeq guard asserts on the key symbol rather than on
GenSeq(...key) on one line, which a two-line refactor walked past (P2-5); the consumer
docstring no longer describes GenSeq as the id source (P2-10); lastPersisted uses the
same CAS as recordIssued (P2-9); the bot-teardown decision not to delete the counter
is written down (P2-4); ModeLegacy, the sentinel-matching contract, and the
unconditional "seedSafetyMargin covers it" docstrings are corrected (P2-6/P2-7, P1-7);
NextEventID rejects a padded robotID rather than trimming it into a different key than
its callers use (P2-12).

Not changed: the .gitignore entries for the other operator tools (P2-1 nit). They are
defensive against exactly the mistake an earlier revision of this PR made — a 41 MB tool
binary committed by git add -A — so they stay.

Latency bound

The allocator runs inside a msgSem slot on saveRobotMessage. 100 held slots
stall fan-out for every bot in the process, which is the hazard that forced the
doorbell off the producer goroutine — except this cannot be asynchronous, since
there is no id to enqueue without it. So it fails fast: 500ms dial / 300ms read,
one retry, ~1s worst case. Giving up fails one enqueue; waiting stalls everyone.

Testing

Run the way CI runs it — -race -shuffle=on -count=1 with a per-package database
reset, matching .github/workflows/ci.yml:

package
pkg/botevent (46 tests, 0 skipped)	ok
tools/botevent-seq (new)	ok
pkg/redis (raw-client chokepoint guard)	ok
modules/robot	ok
modules/bot_api	ok
modules/message	ok
modules/group	pre-existing failures, see below
modules/botfather	pre-existing failures, see below

modules/group and modules/botfather do not pass in my local environment, and they do
not pass on the reviewed head 2f7e58da either — verified in a clean worktree at that
commit, same signatures: 30 s waits ending in a nil dereference (group) and a nil
interface conversion (botfather), which point at a WuKongIM interaction this
environment does not satisfy. modules/robot did fail until I recreated the test
database with utf8mb4_general_ci as CI does; with the right collation it passes.
Reporting this rather than listing the two as green: the packages that exercise this
change (pkg/botevent, pkg/redis, modules/robot, modules/bot_api,
modules/message) are green, and CI is the authority on the other two.

modules/message is named by the brief (it owns the card_action D4 idempotency
claim) and was missing from an earlier revision of this description — review caught
that. The migration was verified applying through real sql-migrate (gorp_migrations
row present, state table created).

A TestMain now fails loudly when MySQL or Redis is missing and CI=true. Every
seqTestCtx call site used to t.Skipf, so a credential or dependency break would have
left only the source guards running while go test exited 0 — indistinguishable from
passing, on the tests that carry the entire safety argument. Locally it still skips.

pkg/botevent runs against a real MySQL and Redis in the standard test database.
An earlier revision used a dedicated one to avoid creating the migration-owned seq
table in a shared schema; that was worse, because CI never creates that database, so
every integration test here silently Skipped — indistinguishable from passing. CI
drops and recreates test before every package, which is what makes creating the
table here safe. Covers concurrent
strict monotonicity, seeding above both ceilings, seed idempotence and
never-lowering, fail-closed on a seed failure induced while INCR stays usable,
pre-activation delegation, activation taking effect with no cache window, and
and draining a quiesced queue through the exclusive cursor at page size 1 (which
proves uniqueness, not the absence of the reordering residual above).

modules/robot, modules/group, modules/bot_api full packages pass, each on a
freshly recreated test database. go build ./..., go vet, gofmt,
make i18n-extract-check, make i18n-lint, git diff --check all clean.

• Unit tests added/updated
• Manually verified (production read-only measurement + local reproduction)

Rollout

0. Close botevent: counter-rollback detection is tolerance-bounded and process-local (activation gate for #697) #704 first. Rollback detection is still tolerance-bounded and
   process-local; both gaps are only reachable after activation, so merging is safe and
   activating is not.
1. Deploy to every replica. Same allocator as before: the mode belief resolves to legacy
   once per process per interval and every allocation goes through GenSeq with no DB
   round trip. One added Redis round trip per allocation — see the note in Summary; it
   is not free, and it is not a new dependency for queue writers.
2. Confirm no pre-fix replica remains. The tool cannot check this and it is
   the gate's whole safety precondition — activating while a legacy replica still
   issues from the bottom of a block is the loss this PR fixes, self-inflicted.
3. Load-test the hottest producer. The added Redis round trip from step 1 is already
   there; activation adds the counter INCR on top, so every allocation is a Redis round
   trip. Neither batching into blocks nor caching the mirror the gate compares against is
   available — both reintroduce the defect.
4. botevent-seq -action activate -yes (it refuses sampled evidence and validates
   the floor against all three sources), then re-run preflight: the duplicate count
   must stop growing. It will not drop — existing duplicates are left
   alone deliberately, since an ack deletes every member sharing a score and there
   is no record of which of a pair was ever delivered.
5. Only after activation is verified, roll out OCTO_BOTEVENT_EXPECTED_MODE=incr so
   a lost mirror and an unreadable authority fail closed instead of degrading.
   Setting it before the flip fails every enqueue closed, so it is last. A malformed
   value now fails closed rather than reading as unset, so a typo cannot quietly disarm
   it.
6. Watch two counters, which mean different things and have different urgencies:
   • dmwork_bot_event_seq_mirror_unauthorized_total — a replica saw a mode mirror the
     authority did not confirm (a forged key, a shared Redis, a restored snapshot). The
     allocator removes the contradicted key and stays on legacy, so this is self-healing
     and the metric is the only signal. botevent-seq -action activate also refuses to
     flip while such a key is present, so this should be zero before step 4.
   • dmwork_bot_event_seq_counter_without_authority_total — a replica found a bot's
     counter while the authority said not-activated, i.e. the authority regressed under an
     activated fleet. This does not self-heal: every enqueue for that bot is refused,
     deliberately, because a GenSeq id there lands below the bot's live cursor. Restore
     the octo_bot_event_seq_state row. Setting OCTO_BOTEVENT_EXPECTED_MODE=incr does
     not unblock it — that flag is a fail-closed assertion, not an override.
   • dmwork_bot_event_seq_high_water_write_failure_total — the durable mark is not
     advancing. Not fatal to delivery, but it is the exposure botevent: counter-rollback detection is tolerance-bounded and process-local (activation gate for #697) #704 owns; see the note under
     Round 6 below.

COMPREHENSION

1. What does this change actually do to the load-bearing path?
   It replaces the id source for every bot event with a strictly monotonic
   counter, and puts one Redis round trip on the hottest producer path where
   GenSeq mostly used none — including before activation, where it is a probe
   rather than an allocation. Until activation the allocator does not change: the
   allocator delegates to GenSeq, so the deploy is a no-op and the switch is a
   separate, reversible-only-forward operator action.

2. What could break because of it (dependents + failure mode)?
   Activating while a pre-fix replica still runs would lose events exactly as
   today — mitigated by the gate plus an explicit manual precondition, not by
   code, which is why step 2 above is stated as a hard gate. A stray Redis key can
   no longer substitute for that gate: the authority is consulted before a positive
   mirror is believed, and the mirror carries the generation the authority named. Seeding below a
   client's existing cursor would make new events unreachable — mitigated by
   seeding from both the queue ceiling and the seq row; the residual case is a
   cursor taken from an already-acked high id combined with a regressed min_seq,
   which the 2×step margin covers for one regression but not for many, and which
   can only be confirmed by observing recovery. A slow Redis now costs the
   fan-out path up to ~1s per allocation, bounded deliberately so a degraded
   Redis fails enqueues rather than exhausting msgSem.

3. How do you know it works (specific test/repro/trace)?
   tools/genseq-repro demonstrates the defect end to end — deterministic
   collision, both skip paths, ack collateral damage — and its assertions are
   mirrored in pkg/botevent against the new allocator where they pass.
   TestNotActivatedDelegatesToLegacy proves the deploy is neutral;
   TestActivationTakesEffectWithoutAProcessCacheWindow proves the flip has no
   divergence window; TestSeedClearsTheLegacyCeilingAtActivation proves the
   first activated id clears a reserved-but-unfinished legacy block;
   TestExclusiveCursorIsLosslessWithMonotonicIDs drains a quiesced queue through
   the real read shape at page size 1 — note it consumes only after production
   finishes, so it proves the allocator's uniqueness and not the absence of the
   reordering residual, which TestKnownResidualZaddReorderingCanStillSkip records
   as still present. TestCounterRollbackIsDetectedAndHealed,
   TestMissingCounterFailsBeforeIssuingFromOne and
   TestMirrorRebuiltFromDBAuthority cover the three Redis-data-loss shapes; the
   production numbers come from read-only SCAN/ZRANGE measurement.

Checklist

• I have read CONTRIBUTING.md
• PR description is in English
• Added tests for my changes
• Updated documentation (task brief under .octospec/)
• Followed commit message conventions (Conventional Commits)

[Jerry-Xin]

The PR is in-scope for octo-server, but the Redis-backed activation/counter durability model can still generate event IDs below bot-held cursors after activation.

🔴 Blocking

• 🔴 Critical: pkg/botevent/seq.go:34 claims Redis RDB rollback is safe because the counter and queue recover together, but bot cursors do not recover with Redis. After activation, a bot can receive or store a high event_id; if Redis later rolls back to an older RDB snapshot, botEventSeq:{robotID} can resume below that client-held cursor. The seeded fast path skips reseeding at pkg/botevent/seq.go:293 and goes straight to INCR, so it can issue below-cursor IDs. If botEventSeq:mode also rolls back or is deleted, pkg/botevent/seq.go:309 and pkg/botevent/seq.go:330 explicitly fall back to legacyEventID, which is the same below-cursor loss mode in reverse. The operator tool only does a Redis SET at tools/botevent-seq/main.go:131, so activation state has the same rollback problem. This needs a durable activation/high-water design, or a fail-closed expected-mode guard plus a floor that cannot roll back below IDs already exposed to clients. Same-Redis co-recovery is insufficient for a cursor-bearing wire contract.

💬 Non-blocking

• 🟡 Warning: The PR includes a committed root-level binary, botevent-seq, which is a 41 MB Linux aarch64 executable with debug info. The source tool exists under tools/botevent-seq/main.go; the built artifact should not be committed to the repo.

✅ Highlights

• The producer migration is broadly consistent: direct queue writers now route through botevent.NextEventID, and addInlineQuery correctly shares the same ID source because it is merged into the same cursor stream.
• The guard tests for GenSeq reintroduction and queue writers are useful and directly target the historical failure mode.
• I ran go test ./pkg/botevent -run 'Test(NotActivated|Activation|Seed|NextEventID|ExclusiveCursor|NoGenSeq|EveryBot)' -count=1; it passed.

[lml2468]

PR Review — PR #702 (Mininglamp-OSS/octo-server)

Reviewed at head d34bdfb055e9, merge-base 40627cc0. Mode: RELIABILITY / DATA-INTEGRITY. Intent: replace the per-process HiLo GenSeq allocator for robotEvent:{robotID} scores — non-monotonic/colliding across 3 replicas, causing permanent event loss under the exclusive cursor + ack-by-score contract — with a per-bot Redis INCR counter behind an activation gate, so merging is behaviour-neutral until an operator flips the mode. Spec: bot-event-score-monotonic/brief.md.
Build/vet/test: go build/go vet (botevent + robot + group + tool) exit 0 ✅. pkg/botevent suite (guards + fake-Scripter unit) PASS; the Redis-backed seq_test.go (real-Redis monotonicity/seed/cursor) is env-gated → CI.

Gate 1 — Spec compliance ✅

Ships exactly the brief's deliverable and honours every load-bearing item: (1) all bot-event producers moved onto the shared NextEventID; (2) the score==event_id equality invariant preserved at each producer; (3) migration one-directionality handled by seeding above max(queue ceiling, legacy seq row); (4) activation-gated, behaviour-neutral merge; (5) does not close #697 (activation + verification is a separate step, correctly stated). No over-build.

Gate 2 — Code quality ✅ (the hard parts are right and tested)

• All producers switched + a source guard that enforces it. NextEventID is the single allocator across modules/robot (enqueueBotEventGeneric, enqueueBotTypedEventGeneric, saveRobotMessage, robot/event.go) and modules/group (both notifyBotJoinedGroup). TestNoGenSeqForBotEventIDs walks the whole repo and fails on any GenSeq(…RobotEventSeqKey|"robotEventSeq:) outside the one allowlisted site — and also asserts seq.go still contains the delegation, so the legacy branch can't be deleted as "dead code" and silently break pre-activation safety. A repo-wide grep confirms legacyEventID is the only GenSeq call for event ids (all other GenSeq calls are unrelated seq keys). This closes the "partial switch = two live id sources" P0.
• Equality invariant preserved. At each producer the same seq is used as both the payload EventID and the ZAdd score (float64(seq)), so ZRemRangeByScore(id,id) ack still targets the right member. Verified at robot/api.go:262 & :323; the rest share the same helper.
• Atomic activation, no mixed-source window. gateSource reads ModeKey and INCRs in one Lua script, so a flip takes effect on the very next allocation everywhere — no per-process mode cache (the file's reviewer-P0). TestActivationTakesEffectWithoutAProcessCacheWindow locks this. The activation tool refuses to flip without -yes + operator confirmation that every replica runs the post-#697 image, and documents why legacy-below-new is the danger and that there is no online deactivate.
• Seed-above-cursor, fail-closed. Seed runs before the first INCR, raises the counter above both the queue ceiling and the frozen legacy seq row (covering the fully-acked-queue-with-live-cursor case), is idempotent + never-lowers (GT Lua), and a seed failure fails the enqueue rather than handing out an unseeded id. Tested: TestSeedRaisesAboveQueueCeiling, TestSeedRaisesAboveLegacySeqRowWithEmptyQueue, TestSeedIsIdempotentAndNeverLowers, TestNextEventIDFailsClosedWhenSeedFails, TestExclusiveCursorIsLosslessWithMonotonicIDs.
• msgSem stall avoided. Tight timeouts (500/300/200 ms) + seqMaxRetries=1 keep a degraded-Redis allocation ~1 s, not ~3 s, so it can't hold msgSem slots and stall fan-out for every bot; failure fails one enqueue (recoverable), matching the ring's rationale.

HIGH — resolve before ACTIVATION (not a merge blocker): the durability argument proves uniqueness, not cursor-monotonicity across an RDB-loss restart

The file's co-recovery section argues an RDB crash is safe because counter C0 and queue max S0<=C0 roll back together, so C0+1 "cannot collide with anything that survived." That is correct — for uniqueness. It does not establish the other invariant the brief calls the hard precondition: new ids must stay above every client-held cursor. Client cursors are external and do not roll back. The two durable floor sources don't help post-activation: the queue max rolls back with the counter (same RDB), and the legacy seq row (MySQL) is frozen at activation and never tracks the post-activation high-water — and the process-local seeded cache means a running replica won't re-seed after a Redis restart anyway, it just INCRs the regressed counter.

Failure scenario: activated bot; snapshot at T0 with counter 49000; events 49001–50000 issued+read, client cursor advances to 49900; Redis restarts from the T0 RDB (prod runs appendonly no, save … 60 10000 → up to 60 s–1 h loss). Counter restores to 49000, queue co-recovers, but the client still holds cursor 49900. New events 49001–49900 land below the exclusive cursor → permanently invisible to that client until the counter re-climbs past 49900 — the exact #697 loss, re-inflicted by a crash. This is strictly worse than GenSeq on this axis, whose min_seq lives in MySQL and does not regress on a Redis crash. It's untested (the suite doesn't simulate counter rollback beneath a live cursor) and unmentioned in the otherwise-thorough durability note.
Fix options before activation: persist a durable, non-co-rolling-back high-water (e.g. periodically advance the seq row past the counter, or floor re-seed to it after a detected restart), or explicitly accept + document the risk with a recovery runbook (after any Redis data-loss restart, counters may sit below live cursors and re-inflict #697 until they climb back). At minimum, correct the co-recovery comment to scope its guarantee to uniqueness.

Coverage / blind spots

Byte-verified: the all-producers-on-NextEventID switch + the repo-wide GenSeq guard, the score==event_id equality at the producers, the atomic gate script, seed-above-cursor ordering + fail-closed, and the msgSem-budget timeouts; ran the pure guard/unit suite green. The real-Redis seq_test.go (monotonicity-under-concurrency, cursor-losslessness) is env-gated → CI. Adversarial checks that did not find a bug: partial-producer-switch (guard blocks it), mode-cache divergence (atomic script), seed-below-cursor (seeded before INCR, tested), ack mis-delete from broken equality (preserved), float64 score precision (ids well inside 2^53). One residual note: clearing ModeKey after activation (unsupported) drops all replicas to legacy, whose lower GenSeq ids would then land below live cursors — worth a one-line "never clear the mode" in the runbook.

Verdict

APPROVE — the merge is behaviour-neutral and the mechanism is correct where it is hardest: every producer is on the single gated allocator with a repo-wide source guard, the score/event_id equality is preserved, the flip is atomic with no mixed-source window, and seeding puts the first activated id above both the queue ceiling and the legacy cursor with a fail-closed path — all locked by tests. Both gates pass for merging. Before activation, close the one real gap: the durability argument guarantees uniqueness but not monotonicity relative to client cursors across an RDB-loss Redis restart, which can re-inflict the #697 invisibility (and is strictly worse than GenSeq on that axis) — resolve with a durable high-water or an explicit accept-and-runbook, and tighten the co-recovery comment. This is a pre-activation reliability item, consistent with the PR's own "does not close the issue / load-test before activation" framing, not a reason to hold the merge.
Suggested decision: Safe to merge (gated, neutral); do not activate until the RDB-restart cursor-regression is resolved or explicitly accepted with a runbook, and CI runs the real-Redis seq_test.go green. merge: MERGEABLE.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Code Review — PR #702 (octo-server)

Summary

This PR replaces the GenSeq HiLo block allocator with a Redis INCR-backed monotonic counter for bot event ID allocation, gated behind an activation switch (botEventSeq:mode = incr). The motivation is well-documented: GenSeq's per-process block allocation produces score collisions under concurrent cold-start or block-extend races, which breaks the consumer's exclusive-cursor pagination and score-based ack. The new allocator seeds the counter above max(queue ceiling, legacy seq row) + 2000 to guarantee forward progress, and the Lua gate script atomically checks mode and increments to avoid a per-process cache window.

Five production call sites (enqueueBotEventGeneric, prepareBotTypedEvent, addInlineQuery, saveRobotMessage, and two notifyBotJoinedGroup variants) are migrated from GenSeq to botevent.NextEventID. Queue key construction is consolidated via botevent.QueueKey(). A source guard test ensures no stale GenSeq call remains. Two operator tools (botevent-seq for preflight/activate, genseq-repro for reproducing the old collision) are included.

Verification

• ✅ All GenSeq call sites migrated — confirmed via diff and the genseq_guard_test.go source guard, which walks the repo and fails if any .go file (except pkg/botevent/seq.go) calls GenSeq with RobotEventSeqKey.
• ✅ Consumer compatibility — getEventsResult at modules/bot_api/events.go:215 uses exclusive Min: "(cursor" pagination and ZRemRangeByScore(id, id) for ack. Both require unique scores; the monotonic INCR allocator guarantees this post-activation.
• ✅ Activation gate atomicity — runGate at pkg/botevent/seq.go:206 runs a Lua script that reads ModeKey and INCRs SeqKey(robotID) in one EVAL call. No TOCTOU window between mode check and allocation.
• ✅ Seed safety margin — seedCounter at :307 raises the counter above max(queueCeiling, legacyCeiling) + seedSafetyMargin (2000 = 2 * legacyGenSeqStep). This clears any score the old allocator could have issued.
• ✅ Co-recovery property — counter (botEventSeq:{robotID}) and queue (robotEvent:{robotID}) live in the same Redis instance/db. An RDB crash restores both atomically; no split-brain on counter vs. queue.
• ✅ Fails closed — if seeding fails, nextEventID returns the error without allocating. The seedFailures atomic counter tracks this for alerting.
• ✅ Inline query parity — addInlineQuery at modules/robot/api.go:1021 shares the allocator with queue events. This is correct: inline query events are merged into the same response stream via inlineQueryEventsMap at :1093-1095 and must not collide.
• ✅ Semaphore safe — saveRobotMessage runs inside a msgSem slot (capacity 100). The allocator's Redis call adds one round-trip (~1ms) inside the slot; the semaphore already bounds concurrency to 100 concurrent listeners.

Static analysis only at head d34bdfb0; build and tests not executed in this environment.

Findings

No P0/P1 issues. Four P2 items and one nit below.

P2 — 42MB compiled binary committed to repo root (botevent-seq:1)

A 42MB ARM64 ELF binary (botevent-seq) is committed at the repository root. This permanently adds ~42MB to every future clone and fetch, is platform-specific (aarch64 only — useless on amd64/macOS dev machines), and build artifacts should not live in source control. The tools/botevent-seq/main.go source is already committed; operators can go build on their target platform. Remove the binary, add botevent-seq to .gitignore, and consider git filter-branch or BFG to purge it from history before merge.

P2 — legacyCeiling may error on missing seq row (pkg/botevent/seq.go:419)

legacyCeiling reads seq.min_seq from the database for the bot's seq key. If the bot has never been assigned a GenSeq block (no row in the seq table), dbr.Load returns an error that propagates up through seedCounter. The inline comment at :371-373 states "bots with no queue and no legacy seq row" should start from 1, but the code does not handle the no-row case — it returns the error, causing seedCounter (and thus nextEventID) to fail. Verify whether octo-lib's Load returns dbr.ErrNotFound for missing rows and handle it explicitly (e.g. return 0 when errors.Is(err, dbr.ErrNotFound)).

P2 — seeded sync.Map grows without bound (pkg/botevent/seq.go:237)

seeded is a process-lifetime sync.Map that caches which robotIDs have been seeded. Every unique bot ID seen by this process is permanently stored with no eviction. On a long-running server serving thousands of bots, this grows unbounded. The memory per entry is small (a string key + struct{}{} value), but consider either: (a) accepting the growth as negligible (a few KB per 10K bots), or (b) using a TTL-based cache or periodic cleanup. Not a blocker — flagging for awareness.

P2 — Plaintext credentials in default DSN (tools/genseq-repro/main.go:35)

The genseq-repro tool's default MySQL DSN is root:demo@tcp(127.0.0.1:3306)/octo?charset=utf8mb4, containing a plaintext password. While this is a developer/operator tool (not production code), committed credentials — even for localhost — set a bad precedent and may trigger secret-scanning alerts. Move to environment variables or require the -mysql flag with no default.

Nit — No operational metric for INCR latency (pkg/botevent/seq.go:206)

The brief recommends a pre-activation load test at 2x peak QPS. Consider adding a Prometheus histogram for per-bot INCR latency so the load-test recommendation has operational follow-through after activation. Without this, there's no way to detect Redis saturation from the allocator in production.

Things I checked that are fine

• Exclusive cursor pagination — the consumer at modules/bot_api/events.go:215 uses Min: fmt.Sprintf("(%d", eventID) (exclusive lower bound). The monotonic allocator guarantees no two events share a score, so no events are skipped or double-delivered.
• Ack-by-score safety — ZRemRangeByScore(key, id, id) at events.go:269 removes exactly the member with that score. With unique scores, this removes exactly one member.
• Queue key consolidation — botevent.QueueKey(robotID) produces robotEvent:{robotID}, matching the existing key format. No data migration needed.
• Test coverage — pkg/botevent/seq_test.go covers: not-activated fallback, activation without cache window, seed clearing legacy ceiling, strict monotonicity under concurrency, seed above queue ceiling, seed above legacy seq row with empty queue, seed idempotency, fail-closed on seed error, and exclusive cursor losslessness.
• Chokepoint guard test — pkg/botevent/chokepoint_guard_test.go is updated to include botevent.QueueKey in the allowed pattern.

Verdict: APPROVED

No correctness, security, or build-blocking issues. The four P2 items are non-blocking: the committed binary is a housekeeping issue (remove before merge if possible), the legacyCeiling no-row case should be verified against octo-lib's Load behavior, the seeded map growth is negligible in practice, and the plaintext DSN is in a developer tool. The core allocator design is sound — atomic Lua gate, proper seeding above both queue and legacy ceilings, co-recovery with the queue, and comprehensive test coverage.

[yujiawei]

Code Review — PR #702 (octo-server)

Reviewed at head SHA d34bdfb055e9ef6f22977c27b0f3900bdd7d5966. I read the full diff, the linked task brief committed in this PR, octo-lib@79f78844bfab config/seq.go, the consumer side in modules/bot_api/events.go, and ran the source guards and go vet locally.

The diagnosis in this PR is excellent and the direction is right: GenSeq is a per-process HiLo allocator and it genuinely cannot satisfy an exclusive-cursor sorted set. The activation gate is the correct shape, and the reasoning quality throughout pkg/botevent/seq.go is unusually high. The blockers below are not about the idea — they are about a red CI check, a gap in the durability argument, and several acceptance criteria from this PR's own brief that are not actually implemented.

---

1. Spec compliance

Spec: ❌

The linked brief (.octospec/tasks/bot-event-score-monotonic/brief.md, added in this PR) is the spec. Most of it is met — the six producers are all migrated, addInlineQuery correctly shares the id source, the queue key is consolidated, the floor-bootstrap and idempotence criteria have tests. Three acceptance items are not.

Missing (漏建)

1. The write-side non-monotonicity counter does not exist. brief.md Acceptance: "新增「score 非单调」计数器（写入时新 score ≤ 队列当前 max 即计数），低基数 label；上线后该计数应恒为 0", restated in D1 ("并在写入侧对「新 score ≤ 队列当前 max」计数告警"). No such counter is in the diff — grep -n 'metric\|prometheus' pkg/botevent/seq.go returns nothing. The only counter added is seedFailures (pkg/botevent/seq.go:243), which is a different signal and is explicitly not wired to Prometheus (seq.go:241-243).

   This is not a bookkeeping omission. It is the only runtime detector for the P0 in §2.1: a counter rollback that puts new ids below client cursors produces no error, no log, and no failed enqueue. preflight cannot see it either, because it counts duplicate scores and this failure mode creates no duplicates. Without this metric, the failure is silent by construction.

2. The migration-verification runbook is not executable as specified. Acceptance: "切换前记录各队列 max score 与 seq 表 min_seq，切换后断言新号段整体高于两者". tools/botevent-seq preflight never reads the seq table at all, and prints maxScore only for queues that already have duplicates (tools/botevent-seq/main.go:117-121). It also defaults to -sample 20 against ~1948 queues, so "各队列" is not covered unless the operator knows to pass -sample 0. The Rollout section in the PR body inherits the same gap.

3. ./modules/message regression not reported. Acceptance names go test -race ./modules/bot_api ./modules/robot ./pkg/botevent ./modules/message -count=1. The PR body reports modules/robot, modules/group, modules/bot_api — modules/message is absent, and it is the package that owns the card_action D4 idempotency claim the brief calls load-bearing.

Added beyond scope (超建)

4. A 42,846,728-byte compiled binary is committed at the repo root. botevent-seq — ELF 64-bit LSB executable, ARM aarch64, with debug_info, not stripped. It is not in .gitignore (git check-ignore botevent-seq → no match) and not in .dockerignore, so Dockerfile:24's COPY . . pulls it into the build context. It does not reach the prod image (that stage copies only /go/release/*), so the cost is not runtime — it is that a 42 MB blob becomes permanent git history in a public repository the moment this merges, and it is an unreviewable executable in a change labelled needs-human-review. This must be removed from the branch (and botevent-seq added to .gitignore) before merge, not after.

Deviation (偏离)

5. brief.md D1 specifies the counter key as botevent:seq:{robotID}; the implementation uses botEventSeq:{robotID} (seq.go:103). Cosmetic on its own, but see §2.7 — the brief's spelling would not have collided with the mode key.

---

2. Code quality

Quality: Changes-Requested

P0

2.1 — pkg/redis guard test fails on this head SHA; the Test CI check is red.

--- FAIL: TestNoRawRedisClientOutsideChokepoint (0.14s)
    chokepoint_guard_test.go:57: raw redis client construction outside the octoredis chokepoint:
          tools/genseq-repro/main.go

tools/genseq-repro/main.go:156 does client := rd.NewClient(&rd.Options{Addr: *redisAddr}). pkg/redis/chokepoint_guard_test.go:19-53 scans every non-_test.go file outside pkg/redis for \b(rd|redis)\.NewClient\(. Reproduced locally in 0.14s; also the cause of the red Test job on run 31000644877.

Two things follow. The fix is one line — route it through octoredis.NewInstrumentedClient, exactly as tools/botevent-seq/main.go:41 already does. But the PR body states "go build ./..., go vet, gofmt … all clean" and lists three module packages as passing; a whole-repo go test ./... was evidently never run, and the brief's regression command does not include ./pkg/redis. Separately, seq.go:245-247 claims the allocator is built through octoredis "so pkg/redis's raw-client chokepoint guard stays satisfied" — true of seq.go, false of the tool shipped alongside it in the same PR.

2.2 — The RDB co-recovery argument does not cover the consumer cursor, which is the one piece of state that is not in Redis.

seq.go:34-47 argues that appendonly no is safe because counter and queue share an RDB domain, so a rollback rewinds both together and "resuming from C0+1 cannot collide with anything that survived." That is correct about collisions and silent about cursors.

Concretely: snapshot at T0 with counter C0 = 5000. Between T0 and the crash, ids 5001–5100 are issued, delivered, and acked; the bot now holds cursor 5100 — in its own process, not in Redis. Redis crashes and restores T0. The counter is back at 5000, so the next 100 events are born at 5001–5100, below the bot's cursor. ZRANGEBYSCORE key (5100 +inf excludes every one of them; they sit in the queue until Robot.MessageExpire (7 days by default) and are never delivered. This is precisely the loss class the PR exists to remove, reintroduced by the fix's own storage choice.

The seeded fast path makes it worse: seq.go:295-299 goes straight to runGate for any bot already in the sync.Map, so no reseed is attempted and the floor is never re-validated. Even if it were, seedCounter could not help — queueCeiling reads the rolled-back queue and legacyCeiling reads a stale min_seq; neither knows the cursor. Bot cursors are entirely client-supplied (modules/bot_api/events.go:77, req.EventID) and never persisted server-side, so this is structural, not an oversight.

Worth noting how the repo already solved the same problem: internal/msgextraseq keeps its activation state in a MySQL row with an epoch, validates the cutover floor against max(DB version, DB seq, observed Redis cursor evidence) and refuses the flip with ErrFloorTooLow otherwise (internal/msgextraseq/activation.go:181-196), and carries a fail-closed expected-mode env guard (store.go:96-116). seq.go:78 describes this design as a "DB-authoritative-style state flag" — but the implementation is a plain Redis key with no epoch, no floor validation, and no expected-mode assertion. The mode key has the same rollback exposure as the counter, and seq.go:302-306 and :330-332 both fail open to GenSeq when it disappears, which is the mirror-image loss (counter-era ids are above min_seq, so legacy resumes below every live cursor) and contradicts this PR's own stated no-fallback principle at seq.go:53-64.

This one was independently raised in the existing review; I confirmed the mechanism against the code and I agree it blocks. It needs either a durable high-water record outside the RDB domain, or a fail-closed guard plus the §1.1 metric so the condition is at least detectable.

P1

2.3 — seedSafetyMargin = 2 × step is justified by an argument that does not hold.

seq.go:127-136 says two steps "covers one reserved block plus one such regression." min_seq regression is not bounded by one step. In octo-lib config/seq.go, each replica's write-back is min_seq = its own CurSeq + seqStep under an unconditional ON DUPLICATE KEY UPDATE. If replica A has advanced to 61000 while replica B is still in the 53000 block, B's next extend writes 54000 — a 7-step regression below the highest id ever issued. The PR's own production data (19 inversions on block boundaries, 6.7-day time regressions) is evidence that replicas drift many blocks apart, so multi-step regressions are the expected case, not the tail.

The queue ceiling normally covers this — but not for the bots most likely to be affected. Robot.MessageExpire defaults to 7 days (octo-lib config/config.go:439) and is applied to the queue key on every enqueue, so any bot idle for a week has an empty queue and queueCeiling returns 0 (seq.go:403-405). For those bots the seed rests on min_seq + 2000 alone, against a client cursor equal to the highest id ever delivered.

Note that the "a larger margin makes it worse" reasoning at seq.go:74-76 is about the un-gated immediate-switch design, where legacy was still issuing from below. Once the gate guarantees no legacy writer, a much larger margin is strictly safer and costs nothing — ids are int64 and exact in float64 up to 2^53. This looks like an argument that survived the design change it was written for.

2.4 — All nine behavioural tests silently skip in CI.

pkg/botevent/seq_test.go:32 points at a botevent_test database. The CI Test job provisions only test (ci.yml:122) and drops/recreates only test between packages (ci.yml:276); nothing ever creates botevent_test. So ctx.DB().Exec("CREATE TABLE IF NOT EXISTS seq …") fails and seq_test.go:60 takes the t.Skipf path. Observed locally on a machine with no MySQL:

--- SKIP: TestNotActivatedDelegatesToLegacy
--- SKIP: TestActivationTakesEffectWithoutAProcessCacheWindow
--- SKIP: TestSeedClearsTheLegacyCeilingAtActivation
--- SKIP: TestNextEventIDIsStrictlyMonotonicUnderConcurrency
--- SKIP: TestSeedRaisesAboveQueueCeiling
--- SKIP: TestSeedRaisesAboveLegacySeqRowWithEmptyQueue
--- SKIP: TestSeedIsIdempotentAndNeverLowers
--- SKIP: TestNextEventIDFailsClosedWhenSeedFails
--- SKIP: TestExclusiveCursorIsLosslessWithMonotonicIDs
ok  github.com/Mininglamp-OSS/octo-server/pkg/botevent  0.083s

Every test carrying the safety argument — fail-closed on seed failure, no-cache-window activation, lossless exclusive cursor at page size 1 — reports ok while running nothing. The tests themselves are genuinely good; they just never execute where it matters, so nothing prevents a regression from landing. The dedicated-database rationale at seq_test.go:22-31 is sound, and the repo already has the pattern that satisfies both constraints: modules/message/issue557_creator_thread_e2e_test.go creates its own isolated database from the CI job's MySQL (see the note at ci.yml:253-255). Doing the same — CREATE DATABASE IF NOT EXISTS botevent_test from the fixture — would make these run in CI without touching the shared migration ledger.

2.5 — The activation drain window is documented only where the operator will never see it.

seq.go:81-83 states the residual window honestly: "a request that has already read legacy and not yet ZADDed when the flip commits; drain writes for a few seconds around the flip to close it." That mitigation appears nowhere else. tools/botevent-seq/main.go:135 prints "activated. Every replica switches on its next allocation." with no mention of draining, and the PR body's Rollout step 4 goes straight from "confirm no pre-fix replica" to -action activate -yes. The unseeded path is where the window lives: seq.go:309 reads the mode with a plain GET outside the gate script, so between that read and the ZADD in the caller the flip can commit and a legacy-numbered id lands in a counter-era queue.

Small window, but the safety property depends on an operator step that only exists in a source comment. Put it in the tool's pre-flip output and in the Rollout section.

P2

2.6 — ModeLegacy is dead and its docstring is false. seq.go:113-115 says it is "written explicitly by the operator tool so preflight can tell 'not yet activated' from 'never configured'". grep -rn 'botevent.ModeLegacy' across the repo returns nothing — tools/botevent-seq only ever writes ModeIncr (main.go:132), and currentMode reports "(unset — legacy)" for a missing key. Either write it in a -action deactivate-marker-style path or delete the constant and the claim.

2.7 — ModeKey shares the per-bot counter namespace. SeqKeyPrefix = "botEventSeq:" (seq.go:103) and ModeKey = "botEventSeq:mode" (seq.go:108), so SeqKey("mode") is ModeKey. Not reachable today — bot ids always carry the _bot suffix (modules/botfather/const.go:20, command.go:955, api_user.go:175) — but the invariant lives in another module and the docstring at seq.go:100-102 reasons only about robotEvent: and robotEventBell:, not about the mode key inside its own prefix. If it ever were reachable, seedSource's tonumber(cur) < floor would evaluate nil < number and error on every allocation for that bot. The brief's botevent:seq:{robotID} spelling (§1.5) avoids this for free.

2.8 — The latency bound is understated, and the expensive path is not the one analysed. seq.go:216-228 reasons about the steady state: one round trip, "~1s worst case". With MaxRetries = 1, DialTimeout 500ms, ReadTimeout 300ms, PoolTimeout 200ms plus go-redis retry backoff, two attempts are closer to ~1.8–2.3s. More importantly the unseeded path — taken once per bot per process, i.e. for every bot after every rollout — does GET mode + ZREVRANGE + a MySQL SELECT (legacyCeiling, seq.go:418-419, no context or timeout) + seed EVALSHA + gate EVALSHA, all inside the same held msgSem slot (modules/robot/api.go:379, cap 100) that the whole bound exists to protect. A stalled MySQL there blocks with no deadline at all. This deserves to be in the analysis and in the rollout step 3 load test, not just the steady state.

2.9 — legacyCeiling's key format is only asserted against itself. seq.go:417 builds "seq:" + common.RobotEventSeqKey + robotID. I verified this matches octo-lib config/seq.go's fmt.Sprintf("seq:%s", flag), so it is correct today. But no test pins it: TestSeedRaisesAboveLegacySeqRowWithEmptyQueue hand-writes the same string in its INSERT (seq_test.go:271), and TestSeedClearsTheLegacyCeilingAtActivation would still pass with legacyCeiling hard-wired to 0, because the three enqueued ids give queueCeiling enough margin to clear its assertion on its own. If octo-lib ever changes the key, legacyCeiling silently returns 0 for every bot — losing exactly the drained-queue protection §2.3 depends on — and the suite stays green. Driving the fixture through ctx.GenSeq(common.RobotEventSeqKey + robotID) instead of a literal INSERT would close it.

2.10 — The GenSeq guard is evadable by a routine refactor. genseq_guard_test.go:31 uses GenSeq\([^)]*(RobotEventSeqKey|"robotEventSeq:), matched line by line. This passes cleanly:

key := common.RobotEventSeqKey + robotID
seq, err := ctx.GenSeq(key)

TestGenSeqGuardWouldCatchAReintroduction only exercises three inline spellings, so it gives false confidence about the escape. There is a strictly tighter guard available: common.RobotEventSeqKey currently appears in exactly one non-test production file (pkg/botevent/seq.go), so asserting the symbol itself appears nowhere else outside the allowlist catches both the inline and the hoisted form. The sibling minKnownQueueWriters = 5 floor in chokepoint_guard_test.go has the matching limitation — it detects a lost writer, not an added one using a new spelling — though its own comments are honest about being a vacuity defence.

2.11 — Every new process bumps the counter by 2000 on first touch of a bot. seedCounter adds the margin whenever floor > 0 (seq.go:388-393), and floor includes queueCeiling. Post-activation steady state, a fresh pod's first allocation for a bot computes queueMax + 2000 and seedSource raises the counter to it. Harmless — monotonic forward jumps never land below a cursor, and int64 has room for centuries of this — but it means the margin is a recurring behaviour, not the "first-time seed" the docstring describes, and it makes the counter= value preflight prints unreliable as a volume signal. Worth either scoping the margin to legacyMax or correcting the comment.

2.12 — Lua/float64 id representation (informational). seedSource round-trips the floor through Lua numbers (doubles) while INCR returns a true int64, and queueCeiling truncates float64 back to int64 (seq.go:406). Exact below 2^53. At realistic magnitudes — min_seq in the 10^5–10^7 range plus 2000 per seed — this is unreachable, so it is a documentation note rather than a defect; I do not think it can produce shared scores in practice.

What is good, and worth keeping exactly as-is

• The activation gate reading the mode inside the allocation script (gateSource, seq.go:177-179) with a deliberate refusal to cache it. The reasoning at seq.go:169-176 is correct and the reason it must not be "optimised" later is stated where the next person will find it.
• Routing addInlineQuery through the same allocator (modules/robot/api.go:1005-1016). The note explaining that a separate sequence would push the shared cursor into the millions is the kind of correction that usually gets silently reverted; recording why the first attempt was wrong is the right call.
• Consolidating producers onto botevent.QueueKey so the seed reads the exact key producers write.
• TestExclusiveCursorIsLosslessWithMonotonicIDs driving the real read shape at page size 1, and tools/genseq-repro as its failing mirror — the observation that a whole-queue read in one pass loses nothing (genseq-repro/main.go:206) is what makes the intermittency legible.

---

3. Overall verdict

REQUEST_CHANGES

Spec ❌ (§1.1–§1.4) and Quality Changes-Requested (§2.1, §2.2). Minimum to clear:

1. tools/genseq-repro/main.go:156 → octoredis.NewInstrumentedClient, so the Test check goes green.
2. Remove botevent-seq from the branch and add it to .gitignore.
3. Add the write-side non-monotonicity counter the brief requires — it is the detector for §2.2.
4. Resolve §2.2: a durable high-water record, or fail-closed on a missing/rolled-back mode key instead of falling through to GenSeq.
5. Make the pkg/botevent tests actually run in CI (§2.4).

4. Suggested direction

For §2.2 specifically, the smallest change that preserves the design is to stop treating a missing mode key as "legacy". Once a process has allocated from the counter it knows activation happened; reverting to GenSeq from that state is never safe, so seq.go:302-306 and :330-332 should return an error rather than delegate. Pair that with an expected-mode assertion at startup, in the shape internal/msgextraseq/store.go:96-116 already uses, so an operator can pin "this build expects incr" and have a rolled-back mode key fail loudly instead of quietly.

For the counter itself, a durable high-water mark — the largest id ever handed out per bot, written outside the RDB domain on a coarse interval (every Nth allocation, not every one) — would let the seed floor survive a rollback without putting MySQL on the hot path. That also gives §2.3 a real bound instead of a 2 × step guess.

For §2.3 in the interim: raise seedSafetyMargin substantially. The "larger is worse" constraint no longer applies now that the gate guarantees no concurrent legacy writer, and int64 headroom is effectively free.

5. Additional notes and coverage gaps

Things a human should verify that I could not, and areas this review did not cover:

• The production Redis configuration is quoted, not verified. appendonly no, save 3600 1 300 100 60 10000, maxmemory 0 / noeviction, single master with 0 replicas — the entire durability argument rests on these, and noeviction in particular is what keeps the counter from being evicted. Please confirm against the live instance, and confirm nothing in the environment can promote a replica or point the client at a different db (pkg/redis/options.go:26-28 and octo-lib config/context.go:107-110 both leave DB unset, so co-location holds today by omission rather than by assertion — a db config field appearing later would break it silently).
• The load test in Rollout step 3 has not been run. The I/O shape change is real and stated plainly by the author: GenSeq served 999 of 1000 allocations with no I/O; every allocation here is a round trip, on the hottest producer path, inside a bounded semaphore. This is the item I would least want to discover in production.
• The three acceptance criteria for the "control must fail" assertions live only in a manually-run tool. tools/genseq-repro is never executed by CI (it only has to compile), so the demonstration that the old allocator loses events can rot without anything noticing.
• ackEvent precision is not tested here. The brief lists "ackEvent 能按 event_id 精确删到目标 member" as acceptance; the consumer is unchanged, so this is pre-existing coverage rather than a regression, but the invariant is not pinned by anything in this PR.
• Not covered by this review: the behaviour of real bot clients' cursor persistence across a 7-day idle window (unobservable server-side, and §2.3's blast radius depends on it); whether the ~1948 production queues include bots whose min_seq has regressed far enough to matter, which only a read-only measurement before activation can answer; and the queue-side Del sites (modules/botfather/command.go:624, :761, :792, modules/robot/api_manager.go:374, modules/botfather/api_user.go:472) which delete the queue but correctly leave the counter — worth confirming that stays deliberate, since deleting the counter there would reintroduce a below-cursor seed on the next allocation.

[yujiawei]

Addendum to my review above — two additional findings, same head SHA (d34bdfb0)

A second pass turned up two mechanisms I did not cover, both of which sharpen §2.2 rather than sitting beside it. The verdict is unchanged; I am adding these now because the first one changes what the fix needs to look like.

---

A1 (P1) — the "counter has been seeded" invariant is cached in a place that cannot observe the counter being destroyed.

gateSource (pkg/botevent/seq.go:177-179) is:

if redis.call('GET', KEYS[1]) ~= ARGV[1] then return -1 end
return redis.call('INCR', KEYS[2])

There is no EXISTS guard on KEYS[2]. Redis INCR on a missing key treats it as 0 and returns 1. The only thing standing between the gate and an unseeded INCR is seeded, a process-local sync.Map (seq.go:237), consulted at seq.go:295-299.

So: the mode key survives, the counter key does not (a partial RDB restore, a key that simply was not in the snapshot, a manual DEL, a FLUSHDB), and the Go process stays up. seeded.Load(robotID) is still true, so seedCounter is skipped entirely, runGate returns 1, and every subsequent event for that bot is numbered from 1 — arbitrarily far below the cursor its client is holding.

This is worse than the rollback case in §2.2. There, the counter falls back to a snapshot value and self-heals after one snapshot window. Here it can drop to 1, so recovery requires re-issuing the bot's entire historical id range before a single event becomes visible again — effectively permanent.

The code already states this exact invariant at seq.go:317-319: "Seed before the first INCR, never after: an id handed out below the floor is exactly the unreachable-event bug." The gap is that it is enforced with process state guarding Redis state. Note also that the maxmemory 0 / noeviction argument at seq.go:49-51 only rules out eviction — it says nothing about partial restore, DEL, or FLUSHDB, so the stated defense is narrower than the threat surface.

Cheap and targeted fix: have the gate distinguish "not activated" from "counter missing" —

if redis.call('GET', KEYS[1]) ~= ARGV[1] then return -1 end
if redis.call('EXISTS', KEYS[2]) == 0 then return -2 end
return redis.call('INCR', KEYS[2])

— and on -2, drop the robotID from seeded and re-seed before retrying. That converts the failure from silent id-space collapse into a re-seed, and it costs one EXISTS on a path that is already one round trip.

---

A2 (P1) — inline-query ids are consumed from the counter but leave no durable trace, so queueCeiling is structurally blind to them.

addInlineQuery (modules/robot/api.go:1020-1038) allocates from the shared counter and then writes only to inlineQueryEventsMap, a plain in-process map[string][]*robotEvent (api.go:349, written at :1036/:1058, read at :1094). It never ZAdds. queueCeiling (seq.go:398-407) reads only the sorted set.

Sharing the allocator here is still the right call — as the comment at api.go:1005-1016 correctly argues, a separate sequence would push the shared cursor into the millions. But it means ids that clients have genuinely observed and paged past exist nowhere durable: not in the ZSet, and not in MySQL. The seed's central assumption — that max(queue ceiling, legacy seq row) bounds everything ever issued — has a hole exactly the width of the inline-query traffic.

Two consequences, one of which does not need a Redis crash at all:

• Combined with A1/§2.2, a re-seed after any counter loss computes a floor blind to every inline id the client already consumed.
• Independently, a plain process restart discards the map while the client keeps its cursor. Pre-activation this is masked (inline ids came from GenSeq too, so min_seq bounds them); post-activation nothing bounds them.

Options, in increasing cost: have addInlineQuery write a per-bot high-water mark key that seedCounter reads alongside the queue ceiling; or ZAdd a tombstone member so the existing ceiling read covers it. The high-water key is probably the smaller change and composes with the durable-high-water direction suggested in §4.

---

Also, for the third time and now escalated to P0 in one input: legacyCeiling does not error on a missing seq row.

Three separate reviewers/passes have now flagged pkg/botevent/seq.go:415-423 as failing closed for bots with no seq row. It does not, and I want the refutation on the record so nobody spends a cycle on it:

• dbr v2.7.5 select.go:426, verbatim: "Unlike Load, it returns ErrNotFound if the SQL result row count is 0." — ErrNotFound is LoadOne's behaviour.
• dbr load.go's for rows.Next() loop leaves count at 0 for zero rows and returns (count, nil).
• octo-lib's own querySeqWithKey (config/seq.go) relies on precisely this: it calls Load(&m) and then tests if seqM == nil to detect "no row". All of GenSeq depends on the nil-error semantics.
• seq.go:419 calls .Load(&minSeq), not LoadOne.

With zero rows, minSeq keeps its zero value and legacyCeiling returns (0, nil), which is the documented intent at seq.go:388-391. No change needed here.

---

One P2 worth folding into the rollout work: seedCounter has no de-duplication, so at activation every concurrent enqueue for a not-yet-seeded bot independently runs queueCeiling (Redis) + legacyCeiling (MySQL) + seedSource. Across ~1948 bots at cutover that is a correlated spike of untimeouted MySQL selects, and it lands on the msgSem-held path discussed in §2.8. A singleflight.Group keyed by robotID around the seed block removes it, and also removes the redundant repeated +2000 jumps from overlapping seeds noted in §2.11.

[Jerry-Xin]

This PR is in-scope for octo-server and addresses a real bot event delivery defect, but the activation state can silently roll back to legacy after Redis data loss/restart.

🔴 Blocking

🔴 Critical — Activation mode is stored only in Redis, but missing mode means legacy allocation.
In pkg/botevent/seq.go, ModeKey read failure to find "incr" delegates to legacyEventID; the seeded fast path also falls back to legacy when the gate script returns -1 at pkg/botevent/seq.go. The operator tool activates by a plain Redis SET at tools/botevent-seq/main.go. With production appendonly no, a Redis restart before the next RDB snapshot can restore a pre-activation state where botEventSeq:mode is absent. At that point all post-fix replicas silently resume GenSeq allocation, issuing IDs below clients that have already observed counter-issued cursors, recreating the exact loss mode this PR is meant to prevent.

There is a second edge on the same failure path: if the operator re-sets ModeKey to "incr" while the server process stayed alive, seeded may still contain the bot ID, so pkg/botevent/seq.go skips seedCounter; if the restored Redis no longer has that bot’s counter key, INCR starts at 1, below the queue/legacy ceiling. The activation gate needs durable state or a restart/reseed-safe protocol before merge.

💬 Non-blocking

🟡 Warning — preflight -sample inspects the first sorted keys only. That is acceptable as an operator convenience, but the default can easily miss the specific hot queues whose duplicate counts matter. Consider making the “verify duplicate count stopped growing” path default to all queues, or require an explicit sample for partial scans.

✅ Highlights

The PR correctly identifies all direct bot-event producers and routes them through a shared allocator, including the inline-query path that shares a cursor with queue events.

The source guards in pkg/botevent are useful and non-vacuous, and go test ./pkg/botevent passes locally.

[lml2468]

PR Review (re-review) — PR #702 (Mininglamp-OSS/octo-server) — REQUEST_CHANGES (flips my APPROVE)

Re-reviewed at new head aedde27a0bbc. This corrects my APPROVE at d34bdfb0. Delta since then: the committed binary is removed and a .gitignore added; pkg/botevent/seq.go is byte-unchanged — the durability gap three reviewers raised is untouched. go build + the pure pkg/botevent guard suite still pass ✅.

Owning two things

1. I missed the 42 MB committed binary. botevent-seq (an unstripped aarch64 ELF) was sitting at the repo root in files.txt and I read past it as the tool directory. mochashanyao, yujiawei, Steve and Jerry-Xin all caught it. It's now fixed: the binary is gone and .gitignore ignores /botevent-seq, /msgextra-version, /card-action-dlq, /genseq-repro, /i18nmarkers. Credit to them; that finding is resolved.
2. I under-ranked the durability gap as "pre-activation, not a merge blocker." On reflection that was wrong, and I'm flipping. I did flag the cursor-regression correctly, but I treated the merge as clean because it's gated. The problem is that the PR's core design justification ships incorrect: seq.go:38-51 and brief D1 present "回退是自洽的，这是选 INCR 的核心理由" as settled — and it is self-consistent only for uniqueness, not for the invariant the brief itself calls the hard precondition (new ids stay above every client cursor). Shipping a load-bearing safety claim that is wrong on the exact failure mode this package exists to remove is a code-quality (Gate 2) defect regardless of the flag, because activation is a one-line SET (tools/botevent-seq/main.go:131) with no code guard against the unsafe condition — the operator relies on that claim. Steve and Jerry-Xin blocked on it; they're right.

Blocking — durability argument proves uniqueness, not cursor-monotonicity across an RDB-loss restart (unchanged at this head)

The co-recovery argument (seq.go:34-47, brief D1): counter C0 and queue max S0≤C0 share the RDB domain, so a crash rolls both back and C0+1 "cannot collide with anything that survived." Correct — for collisions. But client cursors live outside Redis and do not roll back. Prod runs appendonly no, save 3600 1 300 100 60 10000 → a restart loses 60 s–1 h.
Failure scenario: activated bot, snapshot at T0 (counter 49000); events 49001–50000 issued+read, client cursor → 49900; Redis restarts from the T0 RDB. Counter restores to 49000, queue co-recovers, but the client still holds 49900. New events 49001–49900 land below the exclusive cursor → permanently invisible until the counter re-climbs — the #697 loss, self-inflicted by a crash. Strictly worse than GenSeq on this axis, whose min_seq is in MySQL and does not regress on a Redis crash. The two durable floor sources don't help: the queue max co-rolls-back, and the legacy seq row is frozen at activation; and the seeded fast-path (seq.go:293) means a running replica won't re-seed after the restart — it INCRs the regressed counter. If mode also rolls back / is cleared, seq.go:309/:330 fall to legacyEventID, whose lower ids land below live cursors — the mirror-image loss.
Fix (any one, before this is a clean approve): (a) a durable, non-co-rolling high-water — e.g. advance the seq row past the counter periodically and floor re-seeds to it after a detected restart; or (b) detect a counter/queue rollback and re-seed above the last-exposed id; or (c) explicitly scope the claim to uniqueness, add the write-side "new score ≤ observed ceiling" regression counter the brief promised (yujiawei also flagged its absence), and ship a pre-activation runbook covering RDB-restart recovery. At minimum, correct the "回退是自洽的（核心理由）" claim in both seq.go and brief D1.

Still verified-good (unchanged, not re-litigated)

All-producers-on-NextEventID + the repo-wide TestNoGenSeqForBotEventIDs source guard (with the delete-proof allowlist assertion); score==event_id equality at each producer; the atomic gateSource flip with no per-process mode window; seed-before-INCR / fail-closed; the msgSem-protecting timeout budget (500/300/200 ms, 1 retry). These remain correct and well-tested; the mechanism is excellent where it isn't relying on the incomplete durability claim.

Verdict

REQUEST_CHANGES (flipping my d34bdfb0 APPROVE) — the 42 MB binary I missed is fixed (credit mochashanyao/yujiawei/Steve/Jerry-Xin), but the dominant issue is unaddressed at this head: the allocator's core durability rationale guarantees only uniqueness, while an RDB-loss Redis restart drops the counter below surviving external client cursors and re-inflicts the exact #697 invisibility this PR exists to remove — and the shipped code + brief assert that path is safe. Because activation is a bare SET gated only by that (incorrect) claim, this belongs blocked at the PR, not deferred to an operator's judgment. Land a durable high-water (or detect-and-reseed on restart), or scope the claim + add the write-side regression counter + an RDB-restart runbook. Everything else — the all-producers switch, the source guard, the atomic gate, equality, fail-closed, the msgSem budget — is correct and stays.
Suggested decision: Request changes on the RDB-restart cursor-regression (durable high-water or detect-and-reseed) + correct the co-recovery claim; the binary removal + .gitignore is resolved. merge: blocked on the durability fix.

Superseded: re-posting with repository-relative file paths.

[Jerry-Xin]

🔄 Re-review at aedde27a0bbc (prior CR was d34bdfb0 / REQUEST_CHANGES).

The only change since my last review is infrastructural: the committed botevent-seq binary was removed from the repo root and .gitignore now excludes /botevent-seq and the other operator-tool build artifacts (/msgextra-version, /card-action-dlq, /genseq-repro, /i18nmarkers). That resolves my earlier 🟡 note. Every source file is byte-identical to the prior head (pkg/botevent/seq.go, tools/botevent-seq/main.go, modules/robot/event.go, modules/group/service.go, pkg/botevent/seq_test.go all carry the same blob hashes), so the critical durability issue from the last round is unaddressed.

🔴 Blocking

🔴 Critical — Activation state and the counter live only in Redis, and both failure directions still recreate the loss this PR fixes.

• Missing mode ⇒ legacy: ModeKey not equal to "incr" delegates to legacyEventID at pkg/botevent/seq.go:309, and the seeded fast path also falls back to legacy when the gate returns -1 at pkg/botevent/seq.go:295. The operator tool activates with a plain Redis SET at tools/botevent-seq/main.go:132. With production appendonly no, a Redis restart before the next RDB snapshot can restore a pre-activation state where botEventSeq:mode is absent. Every post-fix replica then silently resumes GenSeq allocation, issuing IDs below clients that have already observed counter-issued cursors — the exact unreachable-event loss this PR is meant to prevent.
• Reseed skipped after rollback ⇒ IDs below cursor: the seeded fast path at pkg/botevent/seq.go:295 skips seedCounter when seeded already holds the bot ID. If the operator re-sets ModeKey to "incr" while the process stays alive but the restored Redis no longer has that bot's counter key, INCR starts at 1, below the queue/legacy ceiling. New events land below already-exposed cursors and are permanently unreachable.

The client cursor is external and does not roll back with Redis, so the "counter and queue recover together" durability argument does not cover it. This needs a durable/high-water floor that never rewinds below already-exposed IDs, or a reseed-on-reconnect (drop seeded and re-seed on any counter/mode discontinuity), before merge.

💬 Non-blocking

🟡 preflight -sample inspects only the first sorted keys, so the "verify duplicate count stopped growing" check can miss the specific hot queues that matter. Consider defaulting the verification scan to all queues, or requiring an explicit flag for partial scans.

✅ Highlights

• The botevent-seq binary removal + .gitignore hardening is the right fix for the committed-artifact issue.
• All direct bot-event producers, including the inline-query path that shares a cursor with queue events, correctly route through the shared allocator.
• The source guards in pkg/botevent are non-vacuous and the package tests pass.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Code Review — PR #702 (octo-server)

Reviewer: Octo-Q (automated review)

Summary

This PR replaces the per-process HiLo block allocator (octo-lib GenSeq) for bot event IDs with a Redis INCR-based monotonic allocator, gated behind an explicit activation flag (botEventSeq:mode). The motivation is sound: production measurements show 2624 colliding scores and 19 ordering inversions across replicas, which corrupt the exclusive-cursor pagination and score-based ack in modules/bot_api/events.go. The design is carefully thought through — the activation gate (Lua-atomic mode-check + INCR) prevents the mixed-source defect, the seed logic (max(queue ceiling, legacy min_seq) + 2000) correctly clears both existing queue members and legacy-reserved blocks, and pre-activation behaviour is byte-identical to the old binary. All five production call sites are migrated, guard tests lock in the fix, and an operator tool (botevent-seq) enforces correct rollout ordering.

Verification

Static analysis only at head aedde27a; build and tests not executed in this environment.

• ✅ All 5 production GenSeq call sites migrated — enqueueBotEventGeneric (api.go:262), prepareBotTypedEvent (api.go:323), addInlineQuery (api.go:1021), saveRobotMessage (event.go:357), notifyBotJoinedGroup (group/api.go:1967, group/service.go:2068). Zero remaining GenSeq(RobotEventSeqKey…) calls outside the allowlisted legacy path in seq.go:363.
• ✅ Queue key consolidated — All writers now use botevent.QueueKey(robotID) (= "robotEvent:" + robotID), matching the consumer's robotEventPrefix in modules/bot_api/events.go:32.
• ✅ Lua script atomicity — gateSource reads mode and INCRs in one Redis script (no interleaving). seedSource does compare-and-SET atomically.
• ✅ No fallback after activation — After seed succeeds and mode is incr, Redis failure returns error to caller (fails the enqueue). No silent fallback to GenSeq, which would reintroduce the dual-source defect.
• ✅ Inline query events share the allocator — addInlineQuery (api.go:1021) uses NextEventID, critical because inline and queue events merge into one cursor-sorted stream (api.go:1093-1120).
• ✅ Consumer contract preserved — ZRangeByScore with exclusive (cursor + ZRemRangeByScore(id, id) remain correct under strict monotonicity + uniqueness, which INCR guarantees by construction.
• ✅ Guard tests cover reintroduction — TestNoGenSeqForBotEventIDs scans the codebase and asserts the allowlisted file still contains the legacy delegation (guards against accidental deletion). TestGenSeqGuardWouldCatchAReintroduction proves the regex catches reintroductions and doesn't over-match.
• ✅ Chokepoint guard updated — chokepoint_guard_test.go:35 regex now includes botevent\.QueueKey.
• ✅ Seed idempotency — seedSource Lua only SETs when cur < floor, so concurrent replicas seeding the same bot converge safely.
• ✅ Bounded latency — seqDialTimeout=500ms, seqReadTimeout=300ms, seqMaxRetries=1 caps worst-case allocation at ~1.6s, protecting the 100-slot msgSem from stalling all bot fan-out.

Findings

No P0/P1 issues. Two P2 notes and one nit below.

P2 — legacyGenSeqStep fragile duplicate of unexported octo-lib constant (pkg/botevent/seq.go:115)

legacyGenSeqStep = 1000 mirrors octo-lib's internal config.seqStep, which is not exported. The comment explains the dependency and why it matters (the 2000 safety margin is meaningless without the real value). If octo-lib changes seqStep, this constant silently drifts and the seed margin becomes wrong. Consider exporting seqStep from octo-lib (e.g. config.SeqStep) or adding a runtime cross-check test that asserts the value against actual GenSeq behaviour.

Note — Bot reset path does not clean up botEventSeq: keys (modules/robot/api_manager.go:374, unchanged file)

The admin bot-reset flow deletes robotEvent:{robotID} but not the new botEventSeq:{robotID} counter. This is not a correctness issue — the counter is idempotent, stale values only cost one extra seed round trip, and the seed raises above the existing value anyway. But over many bot lifecycles orphaned counters accumulate. Consider adding client.Del(botevent.SeqKey(robotID)) alongside the existing queue deletion.

Nit — SeqClient singleton uses first-config-wins (pkg/botevent/seq.go:228-237)

sync.Once means subsequent calls with different *config.Config values silently reuse the first client. This matches the existing pattern in the codebase (the bell's ring client uses the same approach) and is correct given the co-recovery invariant (counter must stay in the same Redis as the queue). Flagging for awareness only.

Data Flow Tracing

• Production entry → allocator: All 5 call sites pass (ctx, robotID) to NextEventID. ctx is nil-checked; robotID is TrimSpaced and empty-checked. robotID comes from authenticated context in every path (robot request, group member UID), never from request body.
• Allocator → queue write: Returned seq is used as both sorted-set score (float64(seq) in ZAdd) and payload EventID. Consumer reads score-as-cursor and payload-event_id interchangeably — equality holds because both derive from the same INCR result.
• Seed path data flow: queueCeiling reads ZRevRangeWithScores(key, 0, 0) → max score or 0. legacyCeiling reads SELECT min_seq FROM seq WHERE key=… → value or 0 (no row = 0, bot never issued). floor = max(queue, legacy) + (2000 if floor>0). Seed Lua atomically SETs if current < floor. First INCR returns floor+1 or higher.
• Failure paths: Redis error in gate → error returned → caller fails enqueue (no silent fallback). Seed failure → seedFailures.Add(1) → error returned → caller fails enqueue. Mode cleared after activation → gate returns -1 → falls back to legacyEventID (safe: all other writers would also be in legacy mode).
• Inline query merge: addInlineQuery allocates from same NextEventID, stores in process-local map. getEvents (robot-side, :1085-1120) merges with Redis queue results and sorts by EventID. Same monotonic source → correct merge order.

Blind-spot Checklist (C1–C6, security_sensitive)

• C1 — Dual-path parity: N/A. No symmetric add/remove or create/delete pairs in this PR.
• C2 — Control-flow ordering / nesting reuse: N/A. The allocator is called at discrete points; no nested/double-invocation risk. Lua scripts are Redis-atomic.
• C3 — Authorization boundary ≠ capability boundary: N/A. No new API endpoints. NextEventID is an internal function; robotID is always from authenticated context.
• C4 — Authorization lifecycle / container-member state cascade: N/A. No auth changes.
• C5 — Build/note pass ≠ runtime correctness: N/A. No build artifacts, browser extensions, or packaging changes. .gitignore additions are standard binary exclusions.
• C6 — Governance/policy/doc self-consistency: Clear. .octospec/tasks/bot-event-score-monotonic/brief.md accurately documents the design rationale, activation procedure, and co-recovery argument. Consistent with implementation.

Cross-round Blocker Recheck (R6)

N/A — first review of this PR.

Verdict

The design correctly addresses the root cause of #697 (per-process HiLo blocks producing collisions and ordering inversions across replicas). The activation gate eliminates the mixed-source window, Lua atomicity prevents mode-check races, seed idempotency allows any deployment order, and bounded timeouts protect the msgSem fan-out. All production call sites are migrated, guard tests lock in the fix, and pre-activation deployments are byte-identical to the old binary. The two P2 notes are maintainability/operational-hygiene observations, not correctness concerns.

[Octo-Q] verdict: APPROVE — No P0/P1 findings. Design is sound, implementation is careful, deployment story is safe. Two P2 notes for future consideration.

[yujiawei]

Code Review — PR #702 (octo-server)

Reviewed at head SHA f782959699add7244709aa48cb32a6aeaefe0d6f, merge-base 40627cc0. Note this is one commit past the SHA this review was originally requested for (aedde27a); the branch moved twice while the review was in progress. I re-ran everything against the current head and state below exactly what the two new commits fixed.

I read the full diff, the task brief committed in this PR, octo-lib@79f78844bfab (config/seq.go, config/context.go, pkg/redis/redis.go), the consumer in modules/bot_api/events.go, and .github/workflows/ci.yml. I ran the source guards, go build ./..., go vet, and the pkg/botevent suite locally.

The diagnosis remains excellent and the direction is right: GenSeq is a per-process HiLo allocator and genuinely cannot satisfy an exclusive-cursor sorted set. The activation gate is the correct shape, and the reasoning density in pkg/botevent/seq.go is unusually high. Two blockers from the previous round are now genuinely fixed. The rest still stand, and this round adds three mechanisms that were not previously on the record — one of which changes what "lossless" can mean here.

---

0. What changed since the last review round

Fixed, verified:

1. The committed 42 MB binary is gone — properly. Not just deleted at the tip: botevent-seq appears in no commit of the current branch history (git rev-list 40627cc0..HEAD × git cat-file → absent at all six commits), so the blob will not enter this public repository's permanent history on merge. .gitignore now also covers /botevent-seq, /msgextra-version, /card-action-dlq, /genseq-repro, /i18nmarkers. This was the right way to fix it.

2. The raw-redis-client guard failure is fixed. tools/genseq-repro/main.go now routes through octoredis.NewInstrumentedClient via a new newRedis() helper (tools/genseq-repro/main.go:48-56), and threads RedisAddr into the config. Verified locally at this head:

   ok  github.com/Mininglamp-OSS/octo-server/pkg/redis  0.157s
   

   plus go build ./... and go vet ./pkg/botevent/... ./tools/... clean. The comment added at :50-53 explaining why the tool is in scope for the guard is a good addition.

Everything else below is unchanged code. pkg/botevent/, tools/botevent-seq/, modules/, and .octospec/ are byte-identical to the previously reviewed head — I diffed them explicitly rather than assuming.

---

1. Spec compliance

Spec: ❌

The linked brief (.octospec/tasks/bot-event-score-monotonic/brief.md, added in this PR) is the spec. Most of it is met — the six producers are migrated, addInlineQuery correctly shares the id source, the floor-bootstrap and idempotence criteria have tests. Three acceptance items are still not implemented.

Missing (漏建)

1. The write-side non-monotonicity counter does not exist. brief.md:182 Acceptance: "新增「score 非单调」计数器（写入时新 score ≤ 队列当前 max 即计数），低基数 label；上线后该计数应恒为 0". Re-verified at this head: grep -rn "prometheus\|metric" pkg/botevent/ returns exactly one hit, a comment in bell.go:113. The only counter added is seedFailures (pkg/botevent/seq.go:243), which is a different signal and is explicitly not wired to Prometheus (seq.go:239-243).

   This is not bookkeeping. It is the only runtime detector for §2.1 below: a counter that resumes below client cursors produces no error, no log, and no failed enqueue. preflight cannot see it either — it counts duplicate scores, and this failure mode creates no duplicates. Without this metric the failure is silent by construction.

2. The migration-verification runbook is not executable as specified. brief.md:184-185: "切换前记录各队列 max score 与 seq 表 min_seq，切换后断言新号段整体高于两者". tools/botevent-seq never touches MySQL at all — grep -n "DB()\|min_seq\|MySQL\|NewContext" tools/botevent-seq/main.go returns nothing, so half the required precondition cannot be recorded. It also prints maxScore only for queues that already have duplicates (tools/botevent-seq/main.go:113-116), and defaults to -sample 20 (main.go:34) against ~1948 queues, so "各队列" is not covered unless the operator knows to pass -sample 0. The PR body's Rollout section inherits the same gap.

3. ./modules/message regression not reported. brief.md:180 names go test -race ./modules/bot_api ./modules/robot ./pkg/botevent ./modules/message -count=1. The PR body reports modules/robot, modules/group, modules/bot_api — modules/message is absent, and it owns the card_action D4 idempotency claim the brief calls load-bearing.

Deviation (偏离)

4. brief.md D1 specifies the counter key as botevent:seq:{robotID}; the implementation uses botEventSeq:{robotID} (seq.go:103). Cosmetic alone, but see §3.7 — the brief's spelling would not have collided with the mode key.

5. "Queue keys consolidated onto botevent.QueueKey" is only 4/5 true. The PR body states the keys were consolidated "so the seed reads the exact key producers write." Four writers do (modules/robot/api.go:271, :335, modules/group/service.go:2082, modules/group/api.go:1982). The highest-volume writer does not: modules/robot/event.go:367 still builds fmt.Sprintf("%s%s", rb.robotEventPrefix, robotID), and the entire consumer side keeps its own constant (modules/bot_api/bot_api.go:32, used at events.go:131, :250, :323). Functionally identical today because both literals are "robotEvent:", but the drift this consolidation exists to prevent is still possible in exactly the two places that matter most.

No over-build found beyond item 5's overstatement — the binary that was previously the 超建 finding is gone.

---

2. Code quality

Quality: Changes-Requested

P0

2.1 — After activation, a missing or rolled-back mode key falls open to GenSeq, which is the same data loss mirrored.

seq.go:300-306 (seeded fast path) and seq.go:330-332 (unseeded path):

if v >= 0 { return v, nil }
// Activated, then not: ... Legacy is the safe answer —
// it is what every other writer would now be doing too.
return legacyEventID(ctx, robotID)

Legacy is not the safe answer. Once the counter has issued ids, every counter-era id sits above what GenSeq will hand out next (min_seq was never advanced by the counter), so resuming from GenSeq puts new ids below every live consumer cursor — permanently invisible, which is precisely #697. This also contradicts the file's own stated principle at seq.go:53-64 ("It must NOT quietly reach for GenSeq").

The mode key is a plain Redis SET with no TTL, no epoch, and no durable record (tools/botevent-seq/main.go:131), in an instance running appendonly no. So the trigger is not exotic: a partial RDB restore, a snapshot predating activation, a stray DEL, or a FLUSHDB all produce it, and the process stays up throughout.

The RDB co-recovery argument at seq.go:34-47 is correct about collisions and silent about cursors. Counter and queue rewind together, so ids cannot collide with survivors — but bot cursors are entirely client-supplied (modules/bot_api/events.go:77, req.EventID) and never persisted server-side, so they do not rewind. Snapshot at T0 with counter 5000; ids 5001–5100 are issued, delivered and acked; Redis restores T0; the next 100 events are born at 5001–5100, below the client's cursor 5100, and ZRANGEBYSCORE key (5100 +inf excludes all of them until Robot.MessageExpire (7 days, octo-lib config/config.go:439) reaps them.

The repo already contains the shape that solves this: internal/msgextraseq keeps activation state in a MySQL row with an epoch, validates the cutover floor and refuses with ErrFloorTooLow (internal/msgextraseq/activation.go:181-196), and carries a fail-closed expected-mode env guard (store.go:96-116). seq.go:78 describes this design as a "DB-authoritative-style state flag", but what is implemented is a bare Redis key with none of those three properties.

This was independently raised in the earlier review round and I confirmed the mechanism against the code again at this head. It blocks.

2.2 — gateSource has no EXISTS guard on the counter, and the "already seeded" invariant is held in process memory.

seq.go:177-179:

if redis.call('GET', KEYS[1]) ~= ARGV[1] then return -1 end
return redis.call('INCR', KEYS[2])

INCR on a missing key treats it as 0 and returns 1. The only thing preventing an unseeded INCR is seeded, a process-local sync.Map (seq.go:237) consulted at seq.go:295.

So: the mode key survives, the counter key does not, and the Go process stays up. seeded.Load(robotID) is still true, seedCounter is skipped entirely, runGate returns 1, and every subsequent event for that bot is numbered from 1 — arbitrarily far below its client's cursor. This is strictly worse than 2.1's rollback: there the counter self-heals after one snapshot window; here recovery requires re-issuing the bot's entire historical id range before a single event becomes visible.

The code states this exact invariant at seq.go:317-319 ("Seed before the first INCR, never after") but enforces it with process state guarding Redis state. Note the maxmemory 0 / noeviction argument at seq.go:49-51 rules out eviction only — it says nothing about partial restore, DEL, or FLUSHDB.

Targeted fix, one extra EXISTS on a path that is already one round trip:

if redis.call('GET', KEYS[1]) ~= ARGV[1] then return -1 end
if redis.call('EXISTS', KEYS[2]) == 0 then return -2 end
return redis.call('INCR', KEYS[2])

and on -2, drop robotID from seeded and re-seed before retrying.

P1

2.3 — Ids are monotonic in allocation order, but the queue becomes visible in ZADD order, and the exclusive cursor follows visibility. This residual is not addressed and not disclosed.

Allocation and publication are two separate Redis operations at every producer, e.g. modules/robot/event.go:357 then :368:

seq, err := botevent.NextEventID(rb.ctx, robotID)   // INCR -> N
...
err = rb.ctx.GetRedisConn().ZAdd(key, float64(seq), messageUpdateJson)  // publish N

Interleaving: producer A allocates N and stalls (JSON marshal, GC, a slow ZADD round trip). Producer B allocates N+1, ZADDs, and rings the doorbell (modules/robot/event.go:379, modules/robot/api.go:286). The long-poll consumer added in #685 wakes immediately, reads (cursor +inf (modules/bot_api/events.go:253), receives N+1, and advances its cursor to N+1 (events.go:239-243). A's ZADD then lands at score N. Every subsequent read is (N+1 +inf — A's event is permanently unreachable.

What this PR does fix is real and large: colliding scores, and the multi-day inversions that came from block allocation across restarts. What it does not fix is this window, and the difference matters because:

• The doorbell makes it materially more likely than before, not less: the wake is triggered by the very ZADD that creates the inversion. The justification at modules/robot/api.go:280-284 ("After the ZADD, so a waiter is never woken toward an event the queue does not have yet") reasons only about the ringing producer's own event, not about another producer's in-flight lower id.
• The PR body and brief present the exclusive cursor as becoming lossless once ids are monotonic. modules/bot_api/events.go:186-199 is more careful — it says uniqueness is the assumption and notes that closing the gap "needs a Redis-side allocator or a re-delivery window below the cursor". This PR delivers the allocator, which closes the collision half; the reordering half needs the re-delivery window (or a small lookback below the cursor) and is still open.

I am flagging this as a residual to be stated and scoped, not as a reason to abandon the design. But it should not ship described as lossless.

2.4 — The test named as proof of losslessness cannot observe 2.3.

pkg/botevent/seq_test.go:370-430. Two concurrent producers, then:

wg.Wait()                                  // :397 — all production finished
...
page, err := client.ZRangeByScore(QueueKey(robotID), rd.ZRangeBy{
    Min: fmt.Sprintf("(%d", cursor), Max: "+inf", Count: 1,
}).Result()                                // :409 — consumption starts here

Consumption begins only after every ZADD has landed, so the test asserts that a quiesced sorted set with unique monotonic scores can be drained by an exclusive cursor — very nearly a tautology about ZRANGEBYSCORE. It cannot fail for the reason 2.3 describes. The concurrency in the producers exercises the allocator's uniqueness (valuable, and ZCard at :399-403 is a real assertion), but the test's name and the PR's COMPREHENSION §3 both claim it "drives the real read shape" as an end-to-end losslessness proof, which overstates what it covers. A version that consumes concurrently with production, with an injected pause between one producer's INCR and its ZADD, would fail today.

2.5 — All nine behavioural tests silently skip, in CI and locally.

seq_test.go:32 points at a botevent_test database. The CI Test job provisions and recreates only test (ci.yml:276); nothing anywhere creates botevent_test (grep -n "botevent_test" .github/workflows/ci.yml → no match). So ctx.DB().Exec("CREATE TABLE IF NOT EXISTS seq …") fails and seq_test.go:60 takes t.Skipf. Observed at this head:

--- SKIP: TestNotActivatedDelegatesToLegacy
--- SKIP: TestActivationTakesEffectWithoutAProcessCacheWindow
--- SKIP: TestSeedClearsTheLegacyCeilingAtActivation
--- SKIP: TestNextEventIDIsStrictlyMonotonicUnderConcurrency
--- SKIP: TestSeedRaisesAboveQueueCeiling
--- SKIP: TestSeedRaisesAboveLegacySeqRowWithEmptyQueue
--- SKIP: TestSeedIsIdempotentAndNeverLowers
--- SKIP: TestNextEventIDFailsClosedWhenSeedFails
--- SKIP: TestExclusiveCursorIsLosslessWithMonotonicIDs
ok   github.com/Mininglamp-OSS/octo-server/pkg/botevent

Every test carrying the safety argument reports ok while running nothing. The tests are good; they just never execute where it matters, so nothing prevents a regression from landing. The dedicated-database rationale at seq_test.go:22-31 is sound, and the repo already has the pattern that satisfies both constraints: modules/message/issue557_creator_thread_e2e_test.go creates its own isolated database from the CI job's MySQL (see the note at ci.yml:253-255). A CREATE DATABASE IF NOT EXISTS botevent_test from the fixture would make these run without touching the shared migration ledger.

2.6 — Merging is semantically neutral but not I/O-neutral, and the rollout's load-test gate is therefore in the wrong place.

Pre-activation, seeded is never populated (nothing stores into it on the legacy path), so every allocation reaches seq.go:309:

mode, err := client.Get(ModeKey).Result()
if err != nil && err != rd.Nil { return 0, fmt.Errorf("botevent: read allocator mode: %w", err) }
if mode != ModeIncr { return legacyEventID(ctx, robotID) }

That is one Redis round trip per bot event, on the hottest producer path, inside a held msgSem slot — added at deploy time, before any operator action. GenSeq served 999 of every 1000 allocations with no I/O at all, so the PR body's "Merging is behaviour-neutral" and COMPREHENSION §1's "the deploy is a no-op" are true of the ids and false of the I/O profile and of the failure surface. It also arrives on the allocator's own separate pool and timeout budget (seq.go:223-228), so a degraded Redis can fail this GET — and thus drop an enqueue — in situations where the old binary would have allocated from a process-local block.

Consequence for the plan: Rollout step 3 ("Load-test the hottest producer", positioned between deploy and activation) tests a system that is already carrying the new per-event round trip. The load test needs to happen before the deploy reaches production, or the deploy itself is the experiment.

To be explicit about what I am not asking for: caching the mode is genuinely unsafe, for exactly the reason seq.go:169-176 gives. The fix here is to correct the claim and re-sequence the load test, not to add a cache.

2.7 — seedSafetyMargin = 2 × step rests on an argument that does not hold.

seq.go:127-136 says two steps "covers one reserved block plus one such regression." min_seq regression is not bounded by one step. In octo-lib config/seq.go each replica writes back min_seq = its own CurSeq + seqStep under an unconditional ON DUPLICATE KEY UPDATE, so a replica lagging N blocks writes a value N blocks below the highest id ever issued. This PR's own production data — 19 inversions on block boundaries, 6.7-day maximum time regression — is direct evidence that replicas drift many blocks apart.

The queue ceiling normally covers the gap, but not for the bots most exposed: Robot.MessageExpire defaults to 7 days (octo-lib config/config.go:439) and is refreshed on every enqueue, so a bot idle for a week has an empty queue and queueCeiling returns 0 (seq.go:403-405). For those bots the seed rests on min_seq + 2000 alone, against a client cursor equal to the highest id ever delivered.

The "a larger margin makes it worse" reasoning at seq.go:74-76 is about the un-gated immediate-switch design, where legacy was still issuing from below. Once the gate guarantees no legacy writer, a much larger margin is strictly safer and costs nothing — ids are int64 and exact in float64 below 2^53.

2.8 — Inline-query ids are consumed from the shared counter but leave no durable trace, so queueCeiling is structurally blind to them.

addInlineQuery (modules/robot/api.go:1017-1038) allocates from the shared counter and writes only to inlineQueryEventsMap, a plain in-process map (api.go:349, written at :1036, read at :1094). It never ZAdds. queueCeiling (seq.go:398-407) reads only the sorted set, and nothing writes these ids to MySQL.

Sharing the allocator here is still correct — the comment at api.go:1005-1016 argues that convincingly, and recording why the first attempt was wrong is the right call. But it means ids that clients have genuinely observed exist nowhere durable, so the seed's central assumption (that max(queue ceiling, legacy seq row) bounds everything ever issued) has a hole the width of inline-query traffic. A plain process restart is enough to expose it post-activation; no Redis crash needed. Cheapest fix: have addInlineQuery bump a per-bot high-water key that seedCounter reads alongside the queue ceiling.

2.9 — The activation drain window is documented only where the operator will never see it.

seq.go:81-83 states the residual honestly: "a request that has already read legacy and not yet ZADDed when the flip commits; drain writes for a few seconds around the flip to close it." That mitigation appears nowhere else. tools/botevent-seq/main.go:135 prints "activated. Every replica switches on its next allocation." with no mention of draining, and Rollout step 4 goes straight from "confirm no pre-fix replica" to -action activate -yes. The unseeded path is where the window lives: seq.go:309 reads the mode with a plain GET outside the gate script, so between that read and the caller's ZADD the flip can commit and a legacy-numbered id can land in a counter-era queue. Put it in the tool's pre-flip output and in the Rollout section.

P2

2.10 — ModeLegacy is dead and its docstring is false. seq.go:113-115 says it is "written explicitly by the operator tool so preflight can tell 'not yet activated' from 'never configured'". grep -rn "botevent.ModeLegacy" across the repo returns nothing; tools/botevent-seq only ever writes ModeIncr (main.go:132) and currentMode reports "(unset — legacy)" for a missing key. Either write it or delete the constant and the claim.

2.11 — ModeKey lives inside the per-bot counter namespace. SeqKeyPrefix = "botEventSeq:" (seq.go:103) and ModeKey = "botEventSeq:mode" (seq.go:108), so SeqKey("mode") == ModeKey. Not reachable today — bot ids always carry the _bot suffix (modules/botfather/const.go:20) — but the invariant lives in another module, and the docstring at seq.go:100-102 reasons only about robotEvent: and robotEventBell:, not about the mode key inside its own prefix. If it ever became reachable, seedSource's tonumber(cur) < floor would evaluate nil < number and error on every allocation for that bot. The brief's botevent:seq:{robotID} spelling avoids this for free.

2.12 — The latency bound is understated and the expensive path is not the one analysed. seq.go:216-228 reasons about the steady state: one round trip, "~1s worst case". With MaxRetries = 1, DialTimeout 500ms, ReadTimeout 300ms, PoolTimeout 200ms plus go-redis retry backoff, two attempts land closer to ~1.8–2.3s. More importantly the unseeded path — taken once per bot per process, i.e. for every bot after every rollout — does GET mode + ZREVRANGE + a MySQL SELECT (legacyCeiling, seq.go:418-419, with no context and no timeout) + seed EVALSHA + gate EVALSHA, all inside the same held msgSem slot (cap 100) the bound exists to protect. A stalled MySQL there blocks with no deadline at all. This belongs in the analysis and in the load test.

2.13 — seedCounter has no de-duplication. At activation every concurrent enqueue for a not-yet-seeded bot independently runs queueCeiling (Redis) + legacyCeiling (MySQL, untimeouted) + seedSource. Across ~1948 bots at cutover that is a correlated spike of MySQL selects on the msgSem-held path. A singleflight.Group keyed by robotID around seq.go:320-324 removes it, and also removes the redundant repeated +2000 jumps from overlapping seeds (see 2.15).

2.14 — legacyCeiling's key format is only asserted against itself. seq.go:417 builds "seq:" + common.RobotEventSeqKey + robotID. I verified this matches octo-lib config/seq.go's fmt.Sprintf("seq:%s", flag), so it is correct today. But no test pins it: TestSeedRaisesAboveLegacySeqRowWithEmptyQueue hand-writes the same literal in its INSERT (seq_test.go:271), and TestSeedClearsTheLegacyCeilingAtActivation would still pass with legacyCeiling hard-wired to 0. If octo-lib changes the key, legacyCeiling silently returns 0 for every bot — losing exactly the drained-queue protection 2.7 depends on — and the suite stays green. Driving the fixture through ctx.GenSeq(common.RobotEventSeqKey + robotID) would close it.

2.15 — Every new process bumps the counter by 2000 on first touch of a bot. seedCounter adds the margin whenever floor > 0 (seq.go:388-393), and floor includes queueCeiling. Post-activation, a fresh pod's first allocation for a bot computes queueMax + 2000. Harmless — forward jumps never land below a cursor — but the margin is a recurring behaviour, not the "first-time seed" the docstring describes, and it makes the counter= value preflight prints unreliable as a volume signal.

2.16 — The GenSeq guard is evadable by a routine refactor. genseq_guard_test.go:31 uses GenSeq\([^)]*(RobotEventSeqKey|"robotEventSeq:), matched line by line. This passes cleanly:

key := common.RobotEventSeqKey + robotID
seq, err := ctx.GenSeq(key)

TestGenSeqGuardWouldCatchAReintroduction only exercises three inline spellings, so it gives false confidence about the escape. A strictly tighter guard is available: common.RobotEventSeqKey appears in exactly one non-test production file today, so asserting the symbol appears nowhere outside the allowlist catches both the inline and the hoisted form. (The allowlisted-file-still-contains-the-delegation assertion at :78-87 is a genuinely good idea and should stay.)

2.17 — robotID is trimmed inside the allocator but not at the caller's ZADD. NextEventID does strings.TrimSpace(robotID) (seq.go:279) and derives SeqKey, QueueKey (for the ceiling read) and the seeded entry from the trimmed value, but returns only the id. Callers then use the untrimmed value for the queue key, e.g. modules/robot/api.go:271 key := botevent.QueueKey(robotID). A robotID with surrounding whitespace would allocate against one bot's counter and enqueue into a different key, breaking both the ceiling logic and co-location. Not reachable through today's call sites (ids come from authenticated context / member UIDs), so P2 — but the asymmetry is free to remove: either trim once at the boundary and return the canonical id, or don't trim at all.

2.18 — Numeric upper bound (informational). seedSource passes a Lua number to redis.call('SET', …) (seq.go:158-161), which Redis formats with %.17g. Above ~1e17 that produces scientific notation (1e+17), which INCR then rejects as a non-integer, wedging allocation for that bot permanently; above 2^53 the float64 ZSET score at each producer stops distinguishing adjacent ids, recreating shared scores. Both are unreachable at realistic magnitudes (min_seq in the 1e5–1e7 range plus 2000 per seed), so this is a note rather than a defect — but it is a cliff rather than a graceful degradation, so it is worth a comment.

Verified negatives — please don't spend a cycle re-litigating these

• legacyCeiling does not need to fail on a missing seq row. This has now been raised across multiple rounds. seq.go:419 calls .Load(&minSeq), not LoadOne. dbr v2.7.5 returns ErrNotFound from LoadOne only; load.go's for rows.Next() loop leaves count at 0 for zero rows and returns (0, nil). octo-lib's own querySeqWithKey depends on exactly this (it calls Load then tests if seqM == nil). So legacyCeiling returns (0, nil), which is the documented intent at seq.go:388-391. No change needed.
• Redis topology does not break co-location or the two-key gate script. I checked for this specifically because a multi-key EVAL across botEventSeq:mode and botEventSeq:<id> would CROSSSLOT-fail on every allocation in cluster mode. grep -rn "NewClusterClient\|NewRing\|NewFailoverClient" across octo-lib returns nothing; both octoredis.BuildOptions (pkg/redis/options.go:25-28) and octo-lib's NewRedisCache (config/context.go:107-110) build a single-node rd.NewClient from the same cfg.DB.RedisAddr with DB unset. So co-location holds and the script is safe — by omission rather than by assertion, which is why a future db or cluster config field would break it silently. Worth a guard or at least a comment.

What is good, and worth keeping exactly as-is

• The activation gate reading the mode inside the allocation script (gateSource, seq.go:177-179) with a deliberate refusal to cache it. The reasoning at seq.go:169-176 is correct, and the reason it must not be "optimised" later is recorded where the next person will find it.
• Routing addInlineQuery through the same allocator, and documenting why the first attempt was wrong (modules/robot/api.go:1005-1016). That correction usually gets silently reverted.
• TestNoGenSeqForBotEventIDs asserting that the allowlisted delegation still exists, so the legacy branch cannot be deleted as dead code.
• tools/genseq-repro as the failing mirror of the new tests, and the observation that a whole-queue read in one pass loses nothing — that is what makes the intermittency legible.
• The way the binary was removed: rewritten out of branch history, not just deleted at the tip.

---

3. Overall verdict

CHANGES_REQUESTED

Spec ❌ (§1.1–§1.3) and Quality Changes-Requested (§2.1, §2.2). Credit where due: the two items that made the last round hardest to land — the red guard and the 42 MB blob — are properly fixed, and the blob was removed the right way.

Minimum to clear, in the order I'd do it:

1. §2.1 — stop treating a missing mode key as "legacy" once this process has allocated from the counter; return an error instead. Pair with a fail-closed expected-mode assertion in the shape internal/msgextraseq/store.go:96-116 already uses.
2. §2.2 — add the EXISTS guard to gateSource and re-seed on -2.
3. §1.1 — add the write-side non-monotonicity counter. It is the only detector for both of the above.
4. §2.5 — make the pkg/botevent tests actually run in CI (CREATE DATABASE IF NOT EXISTS botevent_test from the fixture).
5. §2.3 / §2.4 — either add the re-delivery window / cursor lookback, or scope the claim: say plainly that this closes score collisions and leaves the allocate-then-publish reordering window open, and rename the test to what it actually proves.
6. §2.6 — correct "behaviour-neutral" to "id-neutral, not I/O-neutral", and move the load test ahead of the production deploy.

4. Suggested direction

For §2.1 and §2.7 together, the durable high-water mark is the change that retires both: record the largest id ever handed out per bot outside the RDB domain, written on a coarse interval (every Nth allocation, not every one) so MySQL stays off the hot path. That gives the seed a floor that survives a rollback and replaces the 2 × step guess with a real bound. §2.8's inline-query high-water key composes with it directly.

For §2.3, the cheapest honest option is a bounded lookback: read from (cursor - K instead of (cursor and let the client's own idempotency (which D8 already requires for card_action) absorb the re-delivery. That trades a little duplicate traffic for closing the window, and it is the "re-delivery window below the cursor" that modules/bot_api/events.go:196-199 already names as the alternative.

For §2.6, if the pre-activation GET turns out to cost too much under load, the option that does not reintroduce the mixed-source defect is to gate on a process-start read of an expected-mode env var (fail-closed if it disagrees with Redis) rather than a per-allocation read — i.e. accept a restart as the unit of activation instead of a flip. That is a design change, not a tweak, so it belongs in the discussion rather than this round's fix list.

5. Additional notes and coverage gaps

Things a human should verify that I could not, and what this review did not cover. This PR is labelled needs-human-review; these are the items I would actually want a person to check.

• The production Redis configuration is quoted, not verified. appendonly no, save 3600 1 300 100 60 10000, maxmemory 0 / noeviction, single master with 0 replicas — the whole durability argument rests on these. Please confirm against the live instance, and confirm nothing in the environment can promote a replica or point either client at a different db.
• The load test in Rollout step 3 has not been run, and per §2.6 it needs to move earlier. The I/O shape change is real and stated plainly by the author; this is the item I would least want to discover in production.
• §2.3 needs an empirical answer, not just a design one. The window's width in production is a function of ZADD latency on the hottest bot. Someone should measure it before deciding whether the lookback is required or merely prudent.
• I could not execute the nine behavioural tests — I verified they skip (both here and structurally in CI), not that they pass when the database exists. Their assertions read correct to me, but they are unproven at this head.
• tools/genseq-repro is never executed by CI (it only has to compile), so the demonstration that the old allocator loses events can rot silently.
• ackEvent precision is not tested here. The brief lists "ackEvent 能按 event_id 精确删到目标 member" as acceptance; the consumer is unchanged, so this is pre-existing coverage rather than a regression, but nothing in this PR pins it.
• Whether both notifyBotJoinedGroup helpers can fire for one membership addition. modules/group/api.go:1964 and modules/group/service.go:2065 are near-duplicates that now both allocate. I did not trace every caller to rule out double-enqueue; the ids would be distinct so it is not a loss, but it may be a duplicate event.
• Not covered: real bot clients' cursor persistence across a 7-day idle window (unobservable server-side, and §2.7's blast radius depends on it); whether the ~1948 production queues include bots whose min_seq has regressed far enough to matter; and the queue-side Del sites (modules/robot/api_manager.go:374, modules/botfather/command.go:624, :761, :792, modules/botfather/api_user.go:472) which delete the queue but correctly leave the counter — worth confirming that stays deliberate, since deleting the counter there would reintroduce a below-cursor seed on the next allocation.
• Round count. This is the second formal review round, and the head advanced twice during this one. The remaining blockers are concentrated in one file and one design question (durable activation state), so I don't think this needs an escalation yet — but if a third round does not close §2.1/§2.2, I'd stop iterating and settle the durable-high-water design first rather than patching the Redis-key approach again.

[yujiawei]

Addendum to my review above — two further findings, same head SHA (f7829596)

A second pass turned up two items I did not cover. The verdict is unchanged; the first one explains why §1.5 slipped through, which makes it more useful than the finding it supports.

---

A1 (P1) — the queue-writer guard was widened to accept exactly the spellings the consolidation was meant to remove.

§1.5 above notes that modules/robot/event.go:367 — the highest-volume writer — still builds its own queue key instead of calling botevent.QueueKey. The reason nothing caught it is pkg/botevent/chokepoint_guard_test.go:35:

queueKey := regexp.MustCompile(`(robotEventPrefix|"robotEvent:%s"|botevent\.QueueKey|prepared\.QueueKey)`)

The alternation accepts robotEventPrefix and the bare literal "robotEvent:%s" on equal footing with botevent.QueueKey. So a writer that never migrated matches the guard just as well as one that did, and minKnownQueueWriters = 5 (:54) is satisfied either way — the floor counts writers found, not writers consolidated.

The comment directly above it (:32-34) states the opposite intent:

QueueKey helper (#697 consolidated the producers onto it, so the seed reads the exact key producers write…)

The seed does not read "the exact key producers write" as a matter of enforcement — it does so as a matter of coincidence, because robotEventPrefix and QueueKeyPrefix happen to hold the same literal today. This is the same failure shape the sibling TestNoGenSeqForBotEventIDs was written to prevent (a docstring asserting a chokepoint that did not exist), reproduced one file over.

Two things follow. Migrate modules/robot/event.go:367 to botevent.QueueKey, and then narrow the regex to botevent\.QueueKey|prepared\.QueueKey so the guard actually pins the invariant its comment claims. Leaving the alternation wide means the next writer can be added un-consolidated and the suite stays green.

---

A2 (P2) — preflight reads every queue in full, unpaginated.

tools/botevent-seq/main.go:95:

members, err := client.ZRangeWithScores(k, 0, -1).Result()

0, -1 pulls the entire sorted set into the tool's memory in one command, per queue. At the queue sizes described in the PR this is fine. But preflight is precisely the tool an operator reaches for when something has gone wrong — e.g. a consumer outage that let a queue grow unbounded — and that is the case where a single ZRANGE blocks the Redis event loop for the duration and risks OOMing the tool. Both are bad on an instance that is also serving the live fan-out path this PR just made Redis-dependent.

ZSCAN, or ZCARD first with a paginated ZRANGE loop, keeps the diagnostic usable in the situation it exists for. Worth pairing with the -sample 0 gap in §1.2, since the two combine: the fix for §1.2 is "inspect all ~1948 queues", which is also the change that makes an unpaginated full read most likely to hurt.

---

One note on a nearby item, for the record. I checked whether gateSource's two-key EVAL (botEventSeq:mode + botEventSeq:<robotID>) could CROSSSLOT-fail, since that would break every allocation after activation. It cannot today: there is no NewClusterClient / NewRing / NewFailoverClient anywhere in octo-lib, and both clients are single-node from the same cfg.DB.RedisAddr. So this is safe — but safe by omission. If you want it safe by construction, a shared hash tag ({botEventSeq}:mode and {botEventSeq}:<robotID>) costs nothing now and removes the trap for whoever later evaluates a cluster migration. Same category as the DB-unset co-location assumption in §5.

[lml2468]

PR Review (re-review) — PR #702 (Mininglamp-OSS/octo-server) — APPROVE (my blocker fixed)

Re-reviewed at new head 37f45fbdd57e (single forward commit fix(botevent): durable high-water and rollback recovery for the counter on my REQUEST_CHANGES'd f7829596). Delta: seq.go +367, seq_test.go +198. go build/go vet (botevent) exit 0 ✅; pure guard suite passes; the Redis+MySQL rollback/concurrency/high-water tests are env-gated → CI.

My blocker is fixed — the durability argument now proves monotonicity, not just uniqueness

The prior head asserted RDB-crash safety via co-recovery, which only established uniqueness; a rollback dropped the counter below surviving external client cursors and re-inflicted #697. This commit closes it with the two mechanisms it needed, and I byte-verified both:

• Durable high-water in MySQL (seq row botEventHigh:{robotID}, disjoint from GenSeq's namespace). Advanced at most once per highWaterInterval (=1000 ids) via persistHighWater, and folded into every seed's floor in seedCounter (now three sources: queue max, legacy row, durable mark). Because it lives in MySQL it does not share Redis's recovery domain, so it survives an RDB rollback — exactly the property the queue max and the frozen legacy row lack. The write is ON DUPLICATE KEY UPDATE min_seq = GREATEST(min_seq, VALUES(min_seq)) and the mark is v not v+interval — so it can never regress (the GenSeq unconditional-overwrite root cause is explicitly not repeated) and never compounds across seeds. Best-effort (a failed bookkeeping write can't fail the enqueue); a miss only shortens the trail, which seedSafetyMargin (2×step) absorbs. TestHighWaterNeverMovesBackwards pins the GREATEST.
• In-process rollback detection (lastIssued per bot). Every allocation runs checkRegression: v + rollbackTolerance <= high → the counter regressed beneath a long-running (already-seeded) replica that would otherwise keep INCRing the regressed value forever. On detection it re-seeds from the durable floor and retries once; and if the retry is still <= prev, it refuses with an error rather than issue an id below a cursor a client can hold — fail-closed, no silent below-cursor loss. recordIssued advances the per-process max via CAS (concurrent out-of-order arrivals can't overwrite the maximum and manufacture a false regression). TestCounterRollbackIsDetectedAndHealed drives a real regression and asserts detection + above-prior recovery.
• rollbackTolerance (=1000) is justified, not a fudge. Concurrent INCR callers don't observe results in issue order, so a zero-slack check flags ordinary concurrency as a regression — an earlier revision did exactly that and the concurrency test caught a 6000-wide gap. One legacy block is far above any real in-flight count (msgSem ≤100) and far below a real rollback (60–300 s of a live queue). TestNextEventIDIsStrictlyMonotonicUnderConcurrency now also asserts rollbacksDetected==0 under pure concurrency — the no-false-positive guard.
• Mode-rollback mirror loss closed too. ModeKey also regresses under RDB rollback, and dropping to legacy then issues lower GenSeq ids beneath live cursors. ExpectedModeEnv (OCTO_BOTEVENT_EXPECTED_MODE, same shape as #627's expected-mode) makes modeLost fail closed when set to incr instead of silently degrading. runGate is also hardened to reject a zero/negative counter (corrupted restore / manual SET) instead of mistaking it for the -1 not-activated sentinel (now matched exactly via gateNotActivated).

The top-of-file comment is corrected to state plainly that co-recovery buys uniqueness but not monotonicity, and why external cursors force a durable mark.

Residuals (non-blocking, documented)

• Sub-rollbackTolerance regressions (<1000 ids lost) on a live already-seeded replica aren't flagged, so up to ~1000 events for a bot whose cursor sat near the top could be temporarily invisible until the next seed floors to the durable mark. This is the deliberate cost of the concurrency-false-positive tolerance, explicitly documented, bounded, and vastly smaller than the continuous cross-replica loss this PR removes. Acceptable.
• persistHighWater does a DB write ~1/1000 allocations on the saveRobotMessage/msgSem path — best-effort and failure-tolerant, so it can't fail the enqueue, and the amortised cost is negligible against the per-allocation Redis round trip. Fine; worth an eye during the pre-activation load test the PR already mandates.
• ExpectedModeEnv must be rolled out to incr only after activation is verified (a replica expecting incr before the flip fails every enqueue closed) — an operational step, correctly documented.

Verdict

APPROVE (flipping my f7829596 REQUEST_CHANGES) — the durability gap I blocked on is genuinely closed: a durable MySQL high-water mark that survives RDB rollback is folded into every seed floor, in-process rollback detection re-seeds a long-running replica and refuses to issue below a known-held cursor rather than lose events silently, the rollbackTolerance is sized to separate real regressions from concurrency, and the mirror-image mode-rollback path fails closed behind an expected-mode env. The co-recovery claim is corrected to its true scope. Everything I verified before stands (all-producers-on-NextEventID + the repo-wide GenSeq guard, atomic gate with no mode-cache window, score==event_id equality, seed-before-INCR/fail-closed, msgSem timeout budget), and the earlier 42 MB binary is gone with .gitignore. Both gates pass; merge is still behaviour-neutral until activation. Thanks to yujiawei/Steve/Jerry-Xin for corroborating the durability finding — the fix is thorough and test-locked.
Suggested decision: Safe to merge (gated, neutral) once CI runs the Redis+MySQL rollback/concurrency/high-water suites green. At activation: roll OCTO_BOTEVENT_EXPECTED_MODE=incr only after verifying the flip, load-test the hottest producer (now carrying the amortised high-water write), and watch RollbacksDetected. merge: MERGEABLE.

[Jerry-Xin]

One-line summary: The PR is in scope and the allocator design is mostly careful, but the Redis key namespace has a control/data collision that can break allocation for a valid bot ID.

🔴 Blocking

• 🔴 Critical: ModeKey collides with a legal per-bot counter key.
  In pkg/botevent/seq.go:129, counters use SeqKeyPrefix = "botEventSeq:"; in pkg/botevent/seq.go:136, the global mode key is botEventSeq:mode; and pkg/botevent/seq.go:211 makes SeqKey("mode") exactly equal to ModeKey. I did not find a robot ID constraint that makes mode impossible. After activation, a bot with robotID == "mode" would try to seed or increment the allocator mode key as its counter, causing allocation failure and coupling one bot’s data path to the global activation gate. Put the mode key outside the per-bot namespace, or change counters to a non-overlapping namespace such as botEventSeq:counter:{robotID}, and add a regression test asserting SeqKey("mode") != ModeKey.

💬 Non-blocking

• 🟡 Warning: pkg/botevent/seq.go:570 silently drops durable high-water write failures. The rollback-recovery story depends on that mark; at minimum this should be logged/counted so operators know the Redis rollback safety margin has degraded.
• 🟡 Warning: tools/botevent-seq/main.go:34 defaults preflight to a 20-queue sample, but tools/botevent-seq/main.go:118 prints totals that the rollout text treats as validation. For activation verification, defaulting to all queues or labeling sampled output as non-authoritative would reduce operator error.
• 🔵 Suggestion: pkg/botevent/seq_test.go:45 creates the migration-owned seq table directly in the shared test DB. If CI truly recreates the DB per package this is fine, but it is fragile for the documented go test ./... workflow.

✅ Highlights

• The PR is relevant to octo-server: it fixes an existing bot event delivery path.
• The producer conversions consistently route through botevent.NextEventID, including the bot mention Lua commit path via PrepareBotTypedEvent.
• The source guards are useful and I verified the narrow guard tests pass.
• I also verified compile-only runs for modules/group, modules/robot, tools/botevent-seq, and tools/genseq-repro.

Superseded: re-posting with repo-relative paths (no local path leak).

[Jerry-Xin]

Summary: The PR is relevant to octo-server, but the activation gate still has a mixed-source window that can keep a replica on GenSeq after activation under a stale/forged future mirror value.

🔴 Blocking

🔴 Critical — Cached denial can swallow the real activation for the same mirror epoch
pkg/botevent/mode.go trusts a cached negative belief when the current mirror equals b.deniedMirror, and pkg/botevent/mode.go stores that denied mirror value after the authority says legacy. This breaks the “flip never waits for TTL when the mirror claims incr” property when the denied value later becomes the real activation mirror.

Concrete path:

1. Before activation, Redis contains botEventSeq:mode = incr:1 from a stale/forged/manual write.
2. Authority is still legacy, so the allocator denies and caches deniedMirror="incr:1" for negativeBeliefTTL.
3. The operator activates from epoch 0 to epoch 1 in pkg/botevent/state.go, and tools/botevent-seq writes the exact same mirror value incr:1 at tools/botevent-seq/main.go.
4. Until the TTL expires, this process keeps returning decideLegacy because the mirror matches the cached denial, while other replicas can allocate from the counter.

That reintroduces two live id sources on one queue immediately after activation, which is the core loss mode this PR is meant to prevent. The denial cache needs to distinguish “denied against authority epoch X” from a future activation that can legitimately produce the same mirror value, or otherwise force an authority read when the mirror epoch is ahead of the last confirmed legacy epoch. Add a regression test where MirrorValue(1) is denied before Activate() advances epoch to 1.

💬 Non-blocking

🟡 Warning — The pre-activation path is not fully behavior-neutral in I/O shape. Even when delegating to GenSeq, NextEventID now performs a Redis probe before every allocation in pkg/botevent/seq.go. That is materially different from the old GenSeq hot path, especially for inline query events that previously did not need a Redis round trip for ID allocation. It may be acceptable, but the PR description should not say “exactly as before” without qualifying this.

✅ Highlights

The producer-side consolidation onto botevent.NextEventID / botevent.QueueKey is a strong direction, and the guards for GenSeq reintroduction and unrung queue writers are valuable. The MySQL authority plus generation-stamped Redis mirror is also the right architectural shape for avoiding a Redis-only activation state.

Re-posting with repo-relative paths (removed local absolute paths from review body); verdict unchanged.

[Jerry-Xin]

Summary: The PR is relevant to octo-server, and this revision byte-verifiably closes every 🔴 from the previous head (e90540e): the durable-mark bound now fails closed, the queue-key consolidation is complete, and existRobot fails open on lookup error. One new blocking issue remains in the reworked activation-gate cache: a denied mirror value can swallow the real activation for the same epoch.

🔴 Blocking

🔴 Critical — Cached denial can swallow the real activation for the same mirror epoch
pkg/botevent/mode.go:347 trusts a cached negative belief when the current mirror equals b.deniedMirror, and pkg/botevent/mode.go:438 stores that denied mirror value after the authority says legacy. This breaks the "the flip never waits for a TTL when the mirror claims incr" property when the denied value later becomes the real activation mirror.

Concrete path:

1. Before activation, Redis contains botEventSeq:mode = incr:1 from a stale/forged/manual write.
2. Authority is still legacy, so the allocator denies and caches deniedMirror="incr:1" for negativeBeliefTTL (5s).
3. The operator activates from epoch 0 to epoch 1 (pkg/botevent/state.go Activate), and tools/botevent-seq/main.go writeMirror(client, epoch) writes the exact same mirror value incr:1.
4. Until the TTL expires, this process keeps returning decideLegacy because the mirror matches the cached denial, while other replicas allocate from the counter.

That reintroduces two live id sources on one queue immediately after activation — the core loss mode this PR exists to prevent — for up to negativeBeliefTTL. The deniedMirror docstring (pkg/botevent/mode.go:143) claims "a different mirror value, including a genuine activation, is still a conflict"; but a genuine activation at epoch 1 produces the same string that was denied at epoch 0→1, so it is not treated as different. The denial cache needs to distinguish "denied against authority epoch X" from a future activation that can legitimately produce the same mirror value (e.g. key the denial on the observed authority epoch, or force an authority read when the mirror epoch is ahead of the last confirmed legacy epoch). Please add a regression test where MirrorValue(1) is denied before Activate() advances the epoch to 1, and assert the next allocation reads the authority rather than delegating to legacy.

✅ Verified fixed from the previous review round

• Jerry-Xin 🔴 P1-B (bound did not fail closed): persistHighWater now refuses at seedSafetyMargin measured as the span from lastDurable (advances only on a landed write), and afterIssue propagates that error as (0, err) so the enqueue fails — not 1-in-1000. The failure-counter and its non-atomic read-modify-write are gone. Recovery is bounded by a time-budget probe (highWaterProbeEvery), not by traffic.
• yujiawei 🔴 (score-allocator / all ZADD via allocator): both source guards present; chokepoint_guard_test.go now also matches the "robotEvent:" + robotID concatenation spelling that the previous pattern missed, with a self-check test.
• yujiawei 🔴 (QueueKey constant everywhere): rb.robotEventPrefix and the bot_api-local constant are deleted; grep '"robotEvent:' outside tests is empty except the single authoritative QueueKeyPrefix in pkg/botevent/seq.go. All producers and Del sites route through botevent.QueueKey.
• yujiawei 🔴 (existRobot error dropping events): modules/robot/event.go now falls through to prior behaviour on a lookup error (robotID = candidate) and only rejects when the bot is known-not-to-exist.

💬 Non-blocking

🟡 The pre-activation path is not fully behavior-neutral in I/O shape. Even when delegating to GenSeq, NextEventID performs a Redis probe before every allocation (pkg/botevent/seq.go:605), materially different from the old GenSeq hot path — notably for inline-query events that previously needed no Redis round trip for id allocation. Likely acceptable, but the description should qualify "exactly as before".

✅ Highlights

Producer-side consolidation onto botevent.NextEventID / botevent.QueueKey, the GenSeq-reintroduction and unrung-writer guards, and the MySQL authority + generation-stamped Redis mirror are all the right architectural shape.

[yujiawei]

Code Review — PR #702 (octo-server)

Reviewed at head dacb706b239193641965b8c12a1bb3d93ab44f7c, merge base 40627cc0, 24 files / +4953 −90.
This is the sixth formal round on this PR (earlier heads: d34bdfb0, f7829596, 37f45fbd,
2f7e58da, e90540e2). I reviewed the incremental diff e90540e2..dacb706b against the four
findings that blocked the last round, then re-read the whole allocator. Findings were cross-checked
against two independent automated analysis passes; both ran, but one was degraded and I say where
(see Coverage — its silence on the allocator is absence, not clearance).

Real progress this round: the queue-key consolidation is genuinely finished, the existRobot
drop-on-error is gone, the denied-mirror read storm is capped, the cold-process refusal via
EXISTS botEventSeq:counter:{id} is a good and cheap answer to P1-A, and all five P2s from the last
round are closed. The blocking problems below are both in the machinery that was rewritten this
round, which is also the reason for the process note at the end.

---

1. Spec compliance

Spec: ❌

Missing

• .octospec/tasks/bot-event-score-monotonic/brief.md:163-165 still requires a source guard asserting
  that every robotEvent: ZADD site takes its score from the new allocator. The two guards that
  ship assert (a) every queue ZADD rings the doorbell (pkg/botevent/chokepoint_guard_test.go:35)
  and (b) the legacy seq key is not named outside pkg/botevent/seq.go
  (pkg/botevent/genseq_guard_test.go:46). Neither constrains the score source: a writer scoring by
  time.Now().UnixNano() passes both. Last round I said this could move to the follow-up issue
  instead of being built here — it was neither built nor moved (#704's Acceptance section does not
  mention it), so the brief now lists an acceptance criterion nothing tracks. Either add the guard or
  add it to #704 and mark the brief item deferred; I do not care which.

Over-built

• Nothing. Every change in this revision maps to a finding from the last round.

Deviation — all four are documentation that no longer matches the code it describes, in a PR whose
description is the activation runbook:

• pkg/botevent/seq.go:936: "Between the first failed attempt and the span bound there is a full
  interval of grace, so a transient failure costs nothing." False for the first allocation after every
  seed — see P1-1, where it is deterministic rather than occasional.
• PR description, Rollout step 6: "Non-zero means some replica saw a mode mirror the authority did not
  confirm — a forged key, a shared Redis, or a restored snapshot. It self-heals to legacy, so the
  metric is the only signal." Two of the three shapes named in that sentence no longer self-heal.
  legacyDelegate (pkg/botevent/seq.go:733-742) now refuses every enqueue for any bot whose
  counter key survives, and reports it on a different counter,
  dmwork_bot_event_seq_counter_without_authority_total (pkg/botevent/seq.go:505-511), which the
  rollout never mentions. Step 6 needs the new metric and the new failure mode.
• pkg/botevent/seq.go:834 points the reader at noteHighWaterFailure, deleted in this revision.
• unpersistedSpan's docstring (pkg/botevent/seq.go:995-999): "known is false for a bot this
  process has never seeded and never written for." seedCounter stores a base unconditionally at
  pkg/botevent/seq.go:1150 (including 0), so after any successful seed known is always true and
  the base is a DB row value, not something this process wrote. That difference is exactly what
  P1-1 is about.

Everything else I re-checked matches. grep -rn '"robotEvent:' --include=*.go . is now empty outside
tests and the single QueueKeyPrefix definition (pkg/botevent/seq.go:251); the dead
robotEventPrefix field is gone; existRobot errors fall through
(modules/robot/event.go:190-195); the doorbell guard now matches "robotEvent:" + robotID and its
vacuity test asserts each spelling (pkg/botevent/chokepoint_guard_test.go:140-155); TestMain runs
the two real CREATE TABLEs (pkg/botevent/main_test.go:58-66); the expected-mode guard is an atomic
pointer (pkg/botevent/mode.go:243); the CROSSSLOT constraint (pkg/botevent/seq.go:80-86) and the
MySQL ≥ 8.0.16 CHECK assumption
(modules/robot/sql/20260805000001_bot_event_seq_state.sql:48-49) are written down.

---

2. Code quality

Quality: Changes-Requested

P1-1 — the unpersisted-span bound is already over its bound on the first allocation after every seed, so the documented grace does not exist and a failing seq INSERT drops events fleet-wide

Found independently by me and by one automated pass. The arithmetic is deterministic, not a race.

seedCounter records the span base from the durable row, then seeds the counter a full
seedSafetyMargin above a floor that already includes that row:

// pkg/botevent/seq.go:1150
storeMonotonic(&lastDurable, robotID, durableMax)
floor := queueMax
if legacyMax > floor { floor = legacyMax }
if durableMax > floor { floor = durableMax }          // floor >= durableMax
if stateFloor := stateFloorOrZero(ctx); stateFloor > floor { floor = stateFloor }
if floor > 0 {
    floor += seedSafetyMargin                          // pkg/botevent/seq.go:1168
}

So the first id after any seed is >= floor + 1 >= durableMax + seedSafetyMargin + 1, and
unpersistedSpan measures from durableMax. Therefore at pkg/botevent/seq.go:939:

base, span, known := unpersistedSpan(robotID, v)
overBound := known && span >= seedSafetyMargin        // span >= seedSafetyMargin + 1  → always true

overBound is true on the first allocation for every bot in every process, whenever floor > 0.
Post-activation floor > 0 unconditionally, because stateFloorOrZero alone is the validated cutover
floor. At activation itself it is worse: no seq:botEventHigh: rows exist yet, so durableMax == 0
and span equals the whole id (millions).

Four consequences:

• The grace claimed at pkg/botevent/seq.go:936 is not there. The very first failed durable write
  refuses the allocation, instead of after ~2000 ids.
• That refusal loses the event. modules/robot/event.go:389-392 logs a warning and returns; there
  is no retry and no fallback. So a seq-table INSERT that fails or exceeds its 300 ms deadline
  (pkg/botevent/seq.go:963-970) drops bot events for every bot in the process — immediately after
  every deploy or restart, and during the activation flip itself. Reachable without a full MySQL
  outage: the seed's three SELECTs must succeed to get here, so the trigger is a write-side
  problem — failover to a read-only replica, disk full, lock contention on the shared seq table, or
  simply the 300 ms deadline under load.
• It converts the rare probe into a probe storm, which is the hazard the throttle exists to
  prevent. While over the bound, probeDue (pkg/botevent/seq.go:950, highWaterProbeEvery = 200ms
  at :216) lets each bot attempt one 300 ms-deadline INSERT every 200 ms — and it runs inside a
  held msgSem slot. That is ~1.5 slot-seconds per second per actively-producing bot against 100
  slots, so ~67 bots saturate msgSem and stall fan-out for the whole process. The docstring at
  pkg/botevent/seq.go:940-949 names exactly this cliff as the reason the probe stays throttled; the
  per-bot throttle does not bound the fleet. (Raised as a standalone finding by one automated pass; I
  am folding it here because it only becomes reachable because every bot is over the bound.)
• The refusal text is unactionable. unpersistedSpanError (pkg/botevent/seq.go:1027-1035) prints
  base + seedSafetyMargin as the recovery floor. With base == durableMax == 0 an operator reads
  "would land at 2000" for a bot whose live ids are in the millions.

Why the new test cannot see it: pkg/botevent/seq_review_test.go:236-247 performs one healthy
allocation before breaking the write, so lastDurable is set from a successful write and base
equals the real first id. The state the bound exists for — the durable write already failing at seed
time — is never exercised. Its comment ("the first allocation for a bot always does (it is never
throttled)") describes the throttle, which is a different mechanism from the bound.

Minimum fix: measure the span from the same quantity seedCounter would recompute — max(queueMax, legacyMax, durableMax, stateFloor) — rather than from durableMax alone; or store the seeded value as
the base instead of the row. Both keep the inequality on the safe side and restore the grace. Then add
a test whose first allocation happens with the durable write already broken.

P1-2 — invalidateSeeded clears the maps in an order that can leave a bot marked seeded with no durable base, which removes the bound entirely

Raised by one automated pass; I confirmed it and it is a one-line fix.

// pkg/botevent/seq.go:771-778
for _, m := range []*sync.Map{&seeded, &lastPersisted, &lastDurable} {
    m.Range(func(k, _ interface{}) bool { m.Delete(k); return true })
}

Three sequential Range passes, no lock. A concurrent reseed for bot X (pkg/botevent/seq.go:884)
stores lastDurable[X] inside seedCounter and then stores seeded[X]. If that happens after the
seeded pass and before the lastDurable pass, X ends up seeded with no durable base. The window
is not microseconds — it is two full Range passes over every active bot.

The consequence is P1-B reopened. With lastDurable[X] absent, unpersistedSpan returns
known == false, so overBound is false, so persistHighWater returns nil on a failed write
(pkg/botevent/seq.go:952-959, 971-980) — and because nothing ever establishes a base while the writes
keep failing, it returns nil forever. That is precisely the "unbounded ids against a frozen mark"
this round was rewritten to stop, reachable through an ordering rather than through the arithmetic.

Needs mirror loss and a durable-write outage and a concurrent first allocation, so it is
narrower than P1-1 — but the fix is to delete seeded last (a bot should only be treated as seeded
once its base is present), and the property is worth a test that reseeds concurrently with an
invalidation.

P2

1. A denied mirror value can become the genuine activation mirror without changing — raised by
   one automated pass, confirmed. Activate sets epoch = locked.Epoch + 1
   (pkg/botevent/state.go:176), so the first activation from epoch 0 writes exactly incr:1. If a
   leftover or forged incr:1 was already in Redis (a shared Redis whose other tenant activated at
   epoch 1, a restored snapshot, a test key), replicas deny it and cache the denial keyed on that
   value (pkg/botevent/mode.go:347, :434-438). When the operator then activates, the mirror the
   tool writes is byte-identical to the denied one, so those replicas keep serving decideLegacy for
   up to negativeBeliefTTL (5 s) without re-reading the authority — which contradicts
   pkg/botevent/mode.go:44-50 ("propagation of the flip never waits for a TTL"). Blast radius is
   bounded by this round's other fix: bots whose counter already exists hit the counterExists
   refusal, so only bots with no counter yet can take a GenSeq id. At the flip that is every bot, so
   the two fixes cover each other only partially. pkg/botevent/seq_review_test.go:203-215 changes the
   mirror value and never activates the authority, so it cannot see this. Cheap fix: key the denial on
   the observed authority epoch as well as the mirror string.
2. The remediation named in the counterExists refusal does not work — raised by one automated
   pass, confirmed. pkg/botevent/seq.go:737-739 tells the operator to "restore the state row (or set
   OCTO_BOTEVENT_EXPECTED_MODE=incr)". Setting it does not let the process use the counter:
   refreshAuthority reaches assertExpectedMode(false) (pkg/botevent/mode.go:429), which errors
   (pkg/botevent/mode.go:271-275), so the allocation still fails, just with a different message. The
   env guard is a fail-closed assertion, not a manual override. Either drop the parenthetical or say
   what it actually buys ("fail loudly instead of degrading"). This is the one string an operator reads
   during an incident.
3. refreshAuthority's new staleness shortcut can answer from a belief resolved against a different
   mirror observation — found by me and by one automated pass. pkg/botevent/mode.go:377-384 returns
   prior whenever prior != seen, before the conflict rule and before the mirrorClaimsIncr
   handling of an inconclusive read. Interleaving: G1 reads the authority just before the flip and
   installs a negative belief; G2, which already observed the post-flip mirror, is waiting on
   beliefMu, sees prior != seen, and is served decideLegacy — then delegates to GenSeq if that
   bot has no counter yet. It cannot bypass assertExpectedMode (no negative belief can be
   installed while EXPECTED_MODE=incr, pkg/botevent/mode.go:429-431) and it self-corrects on the
   next allocation, so the exposure is one allocation per interleaving — one permanently invisible
   event. I am ranking this P2 rather than P1, against the other pass's call: the window is the
   authority-read latency at a single supervised operator step, and the runbook already prescribes a
   brief write pause there. But TestActivationTakesEffectWithoutAProcessCacheWindow claims the
   property unconditionally, and the fix is one line — re-check the mirror claim against the shortcut
   belief before returning.
4. counterExists is EXISTS and nothing more. No type, value, or provenance check
   (pkg/botevent/seq.go:747), no TTL, and nothing ever removes the key. In a shared or restored Redis
   this wedges a bot's entire event stream for as long as the authority legitimately says legacy — a
   hard failure where the previous behaviour was a metric increment. Accepting that is defensible (it
   is the fail-closed direction), but it should be stated where the shared-Redis case is discussed,
   and it is the second half of the Rollout step 6 correction above.
5. A client-supplied robot_id is still unbounded on the fail-open path — raised independently by
   both automated passes. modules/robot/event.go:190-195 now uses candidate verbatim when
   existRobot errors, which re-opens the growth surface the check was added to close: an
   arbitrary-length client string becomes a no-TTL botEventSeq:counter:{candidate} key under
   noeviction plus a seq:botEventHigh:{candidate} row that is never reclaimed. Failing open was my
   ask last round and I stand by it — dropping events on a DB blip was worse. The resolution that
   satisfies both is a cheap syntactic bound before the lookup (length + charset), consistent with
   NextEventID already rejecting empty and padded ids at pkg/botevent/seq.go:566-570. Reachability
   is low (the attacker must make the query error), which is why this is P2 and not higher.
6. Correlated mirror-loss + unreadable authority still costs one serialized 300 ms read per
   allocation. The error branches at pkg/botevent/mode.go:443-451 return without install, so the
   new prior != seen dedupe never engages and each concurrent allocation pays its own read behind
   beliefMu inside a held msgSem slot. Much narrower than the shape that was fixed (needs Redis and
   MySQL degraded at once) and the enqueues fail anyway, so not blocking — but
   pkg/botevent/mode.go:365-368 claims "one lost mode key costs one authority read rather than one
   per in-flight allocation", which holds only while the authority is readable.
7. The recovery assertion is tight enough to flake. pkg/botevent/seq_review_test.go:284-296
   allows 5 * highWaterProbeEvery (1 s) with a 50 ms poll against a 200 ms probe gate — about five
   real attempts, each a 300 ms-deadline INSERT, after a RENAME TABLE. Under -race -shuffle=on on
   a loaded runner that is thin; 20 * highWaterProbeEvery costs nothing.
8. lastProbe is not cleared by invalidateSeeded (pkg/botevent/seq.go:772) while the other
   three per-bot maps are, and ResetSeededForTest clears all five. Harmless today (a stale timestamp
   delays one probe by <200 ms); noted because the maps are otherwise kept in step.

Raised by the automated passes and adjudicated against

• "pkg/botevent/mode.go:385-394 (if prior != nil && !force) is dead code, because execution only
  reaches it when prior == seen, and resolveMode already evaluated that state." Rejected — do not
  delete it. seen is loaded at pkg/botevent/mode.go:370, after resolveMode's own load at
  :337, so a belief installed between those two loads makes prior == seen while prior is a belief
  resolveMode never evaluated — including an activated one, which that block correctly answers with
  decideCounter. The pass that raised it also could not read pkg/botevent/seq.go (see Coverage), so
  it could not see the other caller of refreshAuthority. Evidence beats assertion here: the claim came
  with no reachable path.
• "The high-water probe should be globally rate-limited rather than per-bot." Accepted, but as a
  consequence of P1-1 rather than as an independent design change — with the span measured correctly,
  bots reach the probe regime only during a genuine prolonged write outage, which is a much smaller
  population. If P1-1 is fixed and the cliff still worries you, a process-wide token bucket is the
  follow-up, not this PR.

---

3. Verdict

CHANGES_REQUESTED — Spec: ❌ and Quality: Changes-Requested; either one blocks.

To be precise about what I am not saying. The diagnosis, the design, and the activation gate are
right, and I am not asking for any of them to change. Everything blocking is post-activation-only, and
the merged state is closer to inert than at any previous head. The two P1s are both in code written
this round, and both are small.

---

4. Suggested split

Fix here:

1. Measure the unpersisted span from the floor seedCounter would recompute, not from durableMax
   alone (P1-1). Then a test whose first allocation for a bot happens with the durable write
   already broken — that is the only shape that catches this class, and it is the third revision in a
   row where the test asserted the mechanism next to the one that was wrong.
2. Delete seeded last in invalidateSeeded, and add a test that reseeds concurrently with an
   invalidation (P1-2).
3. Correct the four documentation deviations in §1, including Rollout step 6 and the
   OCTO_BOTEVENT_EXPECTED_MODE parenthetical in the refusal message (P2-2).
4. Key the denied-mirror cache on the observed epoch as well as the value (P2-1); re-check the mirror
   claim in the staleness shortcut (P2-3). Both one-liners.
5. A length/charset bound on payload.robot_id before the existRobot lookup (P2-5).
6. Either add the score-source guard or record the deferral on #704 and mark the brief item deferred.

Defer: P2-4 (counterExists provenance), P2-6 (correlated-failure read amplification), and a
process-wide probe budget belong with the rest of the rollback-recovery design on #704, which is
already an activation gate.

---

5. Process note — sixth round

This is the sixth review round, and the pattern is worth naming rather than absorbing: rounds 5 and 6
have both found that the previous round's fix to the same mechanism was wrong in a new way. The
durable-mark bound has now been rewritten three times (highWaterFailureLimit → span-from-lastDurable
→ still wrong at the seed boundary), and each revision shipped with a test asserting a neighbouring
property.

The bound is not load-bearing for merging: it is unreachable before activation, and #704 already gates
activation on the rest of the rollback-recovery design. So the recommendation is to stop iterating on it
inside this PR — move the durable-mark bound to #704 in one piece, together with a written statement
of what the recovery floor actually is, and let this PR ship the allocator, the gate, and the
consolidation. That is a plausible one-round path to merge; another local patch to
persistHighWater has now failed twice. This needs a call from whoever owns the change, not another
review round.

---

6. Coverage and blind spots

Stated so the gaps are not read as clearance.

• Tests were not executed. No MySQL or Redis in this environment, so the 46-test integration claim
  is unverified by me. What I did run at this head: go build ./... clean, go vet ./pkg/botevent/...
  clean, and the four dependency-free source guards pass —
  TestEveryBotEventQueueWriterRingsTheDoorbell, TestGuardWouldCatchAnUnrungWriter,
  TestNoGenSeqForBotEventIDs, TestGenSeqGuardWouldCatchAReintroduction. The doorbell guard's
  vacuity floor (minKnownQueueWriters = 5) matches the five production queue ZADD sites exactly, so
  it is tight rather than lax — the last round's concern there is resolved. CI remains the authority on
  modules/group and modules/botfather, which the author reports as failing at both this head and the
  previously reviewed one.
• One of the two automated passes was degraded. It ran and returned, but its input was truncated
  before pkg/botevent/seq.go — it says so itself — so it never saw the allocator, the high-water
  mark, or the Lua scripts. Its silence on those is absence, not agreement; only one pass plus my
  own read covers them. Both passes did independently reach modules/robot/event.go, which is why P2-5
  is credited to both.
• Not examined by anyone this round: tools/botevent-seq/main.go and pkg/botevent/state.go
  (unchanged since the last head, where their gaps were adjudicated as accepted design); Redis
  namespace isolation between environments, which P2-1 and P2-4 both depend on and which is a
  deployment fact, not a code fact; the modules/group / modules/botfather failures; and the
  consumer-side cursor code beyond the docstring change at modules/bot_api/events.go:174-186.
• Noticed, unresolved by anyone, and probably benign: because seedCounter computes
  floor = max(queueMax, legacyMax, durableMax, stateFloor) + seedSafetyMargin and runs on each
  process's first allocation per bot, every process restart raises every active bot's counter by
  ~seedSafetyMargin. Harmless for delivery (the consumer's cursor is exclusive and tolerates gaps) and
  irrelevant for int64, but it is un-stated id-space burn that compounds with restarts, and it
  interacts with the 2^50 floor cap the operator tool enforces. Worth one sentence somewhere.
• For a human to verify, as this PR's needs-human-review label asks: the payload.robot_id
  branch (modules/robot/event.go:183-195) checks bot existence, not authorization, while the
  sibling DM branch in the same function checks creator and friendship (:105, :112). Raised as P1-D
  last round and routed to an owner; still open, still not this PR's job to fix. It stays on the list
  because this PR touches that line.

[an9xyz]

Round 6 addressed at 2b4aac06. Six items; one of them is a decision rather than a
patch, and it is the one that needs your agreement.

The durable-mark bound leaves this PR for #704

@yujiawei's P1-1 is right and the arithmetic is worse than a tuning error. seedCounter
recorded the span base as the durable mark while seeding the counter to
base + seedSafetyMargin, so the span is over its bound on the first allocation after
every seed — deterministically, every bot, every process. The first failed durable write
then refused with no grace, dropped the event, and turned the recovery probe into a storm
inside msgSem slots. And my test could not see any of it, because it did one healthy
allocation first: the state the bound exists for was never exercised. Third revision in a
row where the test asserted the mechanism next to the one that was wrong.

I worked it out before patching again, and a patch cannot fix it:

A seed sets the counter to  S = max(sources) + seedSafetyMargin
A recovery seed replays the same computation over the surviving MySQL sources, so it
lands at S again. Ids issued after a seed are S+1, S+2, … — all above S.

The first id after any seed is already outside what a recovery from an unchanged mark could
reach. The exposure starts immediately, not after a grace period. Closing it requires
advancing the mark at seed time, and a mark that records the seeded value feeds back
into the next seed's floor — so every re-seed compounds by a block, and
TestSeedIsIdempotentAndNeverLowers's no-op property breaks. That is a change to what the
recovery floor is, which is precisely the question #704 already owns for rollback
detection.

So: taken, as both of you recommended. persistHighWater keeps the throttle (re-armed on
failure — the half of P1-7 that stays fixed) and the metric, and says plainly that the
mark trails without bound while writes fail.
TestFailedDurableWriteIsThrottledAndDoesNotFailTheEnqueue asserts the gap is still
there so it becomes the regression test the day #704 closes it, and #704 now carries the
arithmetic plus both failed attempts so nobody tries this road a third time.

This is strictly more honest than either previous revision: one asserted a bound it did not
have, the other over-triggered and dropped events. Neither delivered the property.
@yujiawei — you flagged this as needing an owner's call; that is the call, and the
reasoning is in the code, the brief and #704 rather than only here.

The rest

@Jerry-Xin's 🔴 is correct, and my docstring was the thing that was wrong. I wrote that
"a different mirror value, including a genuine activation, is still a conflict" — but the
first activation produces exactly incr:1, the value most likely to have been forged.
Keying the denial on the observed authority epoch does not help either: the authority is not
re-read, so the two states are indistinguishable from Redis. So the mirror is now removed
rather than remembered — a compare-and-delete on the exact denied value, which is the
symmetric repair to rebuilding it when the authority says activated (when the authority says
legacy, the correct mirror state is absent, since absent is defined to mean "consult the
authority"). No cache, nothing to go stale, and P1-C stays closed because the key is gone
instead of cached. botevent-seq also refuses to flip while such a key is present, which
removes the precondition at the supervised step. Your regression test is in, in the shape you
asked for.

P1-2 — seeded is cleared last; the worst interleaving now leaves a bot unseeded with
stale bookkeeping, which the next allocation fixes.

P2-3 — the staleness shortcut re-checks the mirror claim before serving a negative
belief.

P2-2 — all four corrected, including the one string an operator reads during an
incident. You were right that OCTO_BOTEVENT_EXPECTED_MODE=incr does not unblock a
counterExists refusal; the message now says what the flag actually buys. Rollout step 6
names three metrics and says which one does not self-heal.

P2-5 — a syntactic bound in front of the fail-open lookup: length ≤ the
robot.robot_id column width, no whitespace or control characters. The length bound needs no
judgement call (longer than the column cannot match a row), and I deliberately did not add
a charset allowlist — the column stores whatever it is given, and guessing narrower would
silently stop a real bot's events. A test ties the constant to the column.

Spec — score-source guard recorded as #704 Gap 4 and marked DEFERRED in the brief. Last
round I said "build it or move it" and did neither; that is on me.

P1-D stays with a human owner, unchanged.

Test status

pkg/botevent 45 green with -race -shuffle=on, zero skips under CI=true. pkg/redis,
modules/robot, modules/bot_api, modules/message green. modules/botfather fails in my
local environment and fails identically at dacb706b — verified in a clean worktree at
that commit, same 30 s IM waits and same nil interface conversion. go build ./...,
go vet ./..., golangci-lint on the touched packages, both i18n gates and
git diff --check clean.

On the process note: agreed, and this round is me acting on it rather than agreeing with it.
The pattern you identified — each fix to that mechanism wrong in a new way, each with a test
asserting a neighbour — held for a third round, and the reason turned out to be that the
mechanism's correctness depends on a definition that lives outside it. That is why it moved
whole instead of getting patched again.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Deep Analysis: pkg/botevent/mode.go (586 lines) and pkg/botevent/state.go (209 lines)

Reviewer: Octo-Q (automated review)
Head SHA: 2b4aac0

---

1. What Each File Does

state.go — The DB-authoritative activation state for the monotonic bot event ID allocator. It owns a singleton row in octo_bot_event_seq_state with three fields: mode (0=legacy, 1=incr), epoch (generation counter, bumped on each activation), and cutover_floor (minimum ID the counter must start above). It provides:

• ReadState / ReadStateContext — read the singleton row, distinguishing ErrStateMissing (table or row absent = pre-migration) from other errors (DB unreachable).
• Activate — atomic flip from legacy to incr under FOR UPDATE, validating floor > observedMax before committing.
• stateFloorOrZero — best-effort read of cutover_floor for use as a seed floor source.

mode.go — The per-process mode decision engine. It decides, for each allocation, whether to use the Redis counter (decideCounter) or delegate to legacy GenSeq (decideLegacy). It implements a cached "belief" system with asymmetric trust rules:

• A positive belief (activated) is terminal — never downgraded, even on DB errors.
• A negative belief (not activated) is cached for negativeBeliefTTL (5s) but overridden immediately if the Redis mirror claims activation (forcing a fresh DB read).
• An epoch-stamped mirror value (incr:{epoch}) prevents hand-written or stale mirrors from opening the gate.

---

2. Activation Gate Logic

The gate is a two-layer system:

Layer 1 — Authority (MySQL). Activate() in state.go:131-193 performs a SELECT ... FOR UPDATE then a conditional UPDATE ... WHERE mode=0. The floor is validated against observedMax before the flip. The epoch is bumped atomically. This is called by the tools/botevent-seq operator CLI, not by the allocator itself.

Layer 2 — Mirror (Redis) + Belief cache (process-local). The allocator's hot path reads the Redis mirror via probeAllocatorState (seq.go:749), then calls resolveMode (mode.go:382) which:

1. If positive belief cached -> return decideCounter immediately (line 389-390).
2. If negative belief within TTL and mirror does not claim incr -> return decideLegacy (line 391-393).
3. If mirror claims incr but belief is negative -> force DB read via refreshAuthority (line 400).
4. refreshAuthority reads the DB, and if activated, installs a positive belief with the validated epoch.

The gate script itself (gateSource, seq.go:268-271) atomically compares the live Redis mirror against the exact validated string incr:{epoch} and INCRs only if they match. This means even if the belief cache is stale, the Lua script catches a mirror change.

What happens if activation fails partway?

Activate() uses a proper transaction with defer tx.RollbackUnlessCommitted() (state.go:139). The flip is a single conditional UPDATE — if RowsAffected != 1, it returns an error (line 184). There is no partial state possible at the DB level.

However, the mirror write happens later, on the allocator side, not inside Activate. In seq.go:636-642, after seeding the counter, the mirror is written via client.Set(ModeKey, b.mirrorValue(), 0). If this write fails:

• The DB says activated, the Redis mirror is absent/wrong.
• The next allocation from any replica reads the mirror (absent), calls resolveMode, which calls refreshAuthority, reads the DB, finds activated, and rebuilds the mirror (seq.go:627-642).
• During this window, every allocation costs an extra DB read, bounded by negativeBeliefTTL.

This is a documented, self-healing gap — not a bug. The design is sound.

---

3. Concurrency Issues

3a. beliefMu lock ordering — CLEAR (no issue found).

beliefMu (mode.go:173) is held only during refreshAuthority. It is never held while acquiring seedMutex or any Redis lock. The seedLocks map (seq.go) is per-bot and never held during a beliefMu acquisition. No lock ordering violation.

3b. activeBelief atomic pointer — CLEAR.

Read via activeBelief.Load() on the hot path without locks. Written only inside refreshAuthority (under beliefMu) or noteMirrorRepairFailed (under beliefMu). Copy-on-write semantics: a new belief struct is created and the pointer is atomically swapped. No reader can see a half-updated belief. This is correct.

3c. Double-check under lock — CLEAR.

refreshAuthority (mode.go:413-445) captures seen before locking, then re-reads activeBelief as prior after acquiring beliefMu. If prior != seen, someone else refreshed. The logic correctly distinguishes:

• prior.activated -> return counter (line 424)
• !mirrorClaimsIncr -> return legacy (line 427)
• Mirror claimed incr but prior is negative -> fall through and re-read (line 429)

This is correct. The third case avoids serving a stale negative belief when the caller observed a mirror claiming activation.

3d. noteMirrorRepairFailed under beliefMu — P2 (minor concern).

At mode.go:539-553, noteMirrorRepairFailed acquires beliefMu to do a copy-on-write update of the belief. This is called from dropUnauthorizedMirror (seq.go:785) which is called from allocate (seq.go:612). The call path does NOT hold beliefMu at that point, so there is no deadlock risk. However, noteMirrorRepairFailed acquires beliefMu while refreshAuthority also holds it — these are serialized, not nested. Clear.

3e. sync.Once for seqClientOnce — CLEAR.

seqClientOnce (seq.go:339) is a standard sync.Once for client construction. No misuse.

3f. seeded sync.Map invalidation ordering — CLEAR with documented risk.

invalidateSeeded (seq.go:798-809) clears lastPersisted before seeded, which is the correct order (documented in the comment at line 798). Clearing seeded first would let a concurrent reseed's marker survive while its companion state was wiped. This is correct.

3g. Race between resolveMode and concurrent activation — CLEAR.

A concurrent activation changes the Redis mirror and DB row. resolveMode reads the mirror first (via probe), then decides. If the mirror changed between probe and gate, the Lua gate script catches it (returns -1). If the DB changed between probe and refreshAuthority, the DB read returns the new state. The system converges.

---

4. State Persistence

4a. How is state stored?

• Authoritative state: MySQL singleton row in octo_bot_event_seq_state. Survives Redis restarts, RDB rollbacks, and process restarts. Read with a 300ms deadline (authorityTimeout, mode.go:155).
• Mirror: Redis key botEventSeq:mode with value incr:{epoch}. No TTL (set with expiration 0). Regresses with RDB snapshots (appendonly no in production).
• Process-local belief: activeBelief atomic pointer. Lost on process restart.

4b. Can state become stale?

• Negative belief: Stale for up to negativeBeliefTTL (5s). But a mirror claiming activation forces an immediate re-read, so the only stale window is when the DB says activated but the mirror hasn't been written yet.
• Positive belief: Never stale in the downward direction (terminal). Can be stale in the upward direction if epoch changes — but the gate script compares the exact epoch string, so a stale epoch closes the gate rather than allocating.
• Mirror: Can regress with an RDB snapshot. The design explicitly handles this: an absent or mismatched mirror triggers a DB read and rebuild.

4c. Can state be cleared?

• ResetModeBeliefForTest() (mode.go:563) clears the process-local belief.
• ResetSeededForTest() (seq.go:1172) clears all process-local caches including belief.
• There is no production mechanism to clear a positive belief. This is by design: once activated, the process never returns to legacy.
• The mirror can be deleted (by dropUnauthorizedMirror or manually), which triggers a DB re-read on next allocation.

---

5. Error Handling Gaps

5a. stateFloorOrZero silently swallows all errors — P2 (acceptable, documented).

At state.go:200-208, any error from ReadStateContext is silently converted to return 0. This is documented as "best-effort" and the comment explains that other floor sources cover the cases. The function is only used as one of four floor sources in seedCounter, so a zero return degrades safety margin but does not cause incorrect behavior.

5b. readStateDeadlined creates a new background context — P2 (acceptable).

At mode.go:556-559, readStateDeadlined uses context.Background() as the parent, not the caller's context. This is deliberate (documented in the authorityTimeout comment at mode.go:149-155): the caller may be inside a msgSem slot with no deadline, and an unbounded wait would stall fan-out. The 300ms timeout is the bound.

5c. Activate does not validate floor > 0 — P2 (minor).

At state.go:131-193, Activate validates floor > observedMax but does not reject floor <= 0. A zero or negative floor would be stored in cutover_floor and used by stateFloorOrZero as a seed floor source. Since seedCounter (seq.go:1133) only adds seedSafetyMargin when floor > 0, a non-positive floor would effectively be ignored. Not a bug, but the operator tool should validate this.

5d. Inconclusive authority read caches the negative answer — CLEAR (correct).

At mode.go:497-506, when the authority is unreadable and no prior positive belief exists, a negative belief is cached. This prevents a DB outage from causing one failed read per allocation. The TTL is negativeBeliefTTL (5s), so recovery is automatic.

---

6. Data Flow Trace

DB row -> runtime decision:

1. Operator runs botevent-seq activate -> calls Activate(ctx, floor, observedMax) -> DB row updated: mode=1, epoch=N, cutover_floor=floor.
2. Operator writes Redis mirror: SET botEventSeq:mode incr:N (done by the tool, not by Activate).
3. Next allocation on any replica: allocate() calls probeAllocatorState() -> reads mirror incr:N and counter existence via Lua.
4. resolveMode(ctx, "incr:N") -> parseMirror extracts epoch N -> no prior belief or negative belief -> calls refreshAuthority.
5. refreshAuthority calls readStateDeadlined -> DB returns mode=1, epoch=N -> assertExpectedMode(true) passes -> installs belief{activated: true, epoch: N}.
6. Back in allocate: rebuildMirror is true if mirror was absent/wrong -> invalidateSeeded() -> reseed() -> client.Set(ModeKey, "incr:N", 0).
7. allocateFromCounter -> runGate -> Lua checks GET ModeKey == "incr:N" and EXISTS SeqKey -> INCRs counter -> returns ID.
8. afterIssue -> checkRegression -> recordIssued -> persistHighWater.

Stale data paths:

• If mirror is lost (RDB rollback): gate returns -1 -> gateClosed -> forced refreshAuthority -> re-reads DB -> rebuilds mirror.
• If DB regresses (manual UPDATE/restore): positive belief is terminal, so existing processes keep allocating. A cold process reads DB, finds legacy, but legacyDelegate checks counterExists (from probe) and refuses if the counter is present.
• If both mirror and counter are lost (Redis flush + DB regression): cold process has no evidence. Only OCTO_BOTEVENT_EXPECTED_MODE=incr closes this residual window.

---

7. Mode Transitions

Valid transitions:

• legacy -> incr: Via Activate(). Epoch bumped. Floor validated. Irreversible at the process level (positive belief is terminal).
• incr -> incr (re-activation): Activate() returns flipped=false with existing epoch (state.go:165). Idempotent.
• incr -> legacy: Only by manual DB manipulation (UPDATE/restore). The system detects and resists this: existing processes keep allocating (positive belief), cold processes refuse if counter exists (legacyDelegate).

Can the system get stuck?

No. The system cannot get stuck in an invalid state:

• A positive belief cannot be downgraded, so an activated process never returns to legacy.
• A negative belief expires after 5s, so a newly activated system converges within one TTL.
• The repair cooldown (1s) prevents a stuck unauthorized mirror from causing a read storm, and it expires quickly.
• maxAllocAttempts = 3 bounds mutual recursion between afterIssue and gateClosed.

The one "stuck" scenario is a Redis that keeps losing state (FLUSHDB loop), which hits the attempt limit and returns an error. This is the correct behavior — the caller sees a failure rather than an infinite loop.

---

8. Logic Bugs and Findings

Finding 1 — P2: Activate epoch overflow not guarded.

state.go:176: locked.Epoch+1 with no overflow check. Epoch is uint64, so overflow requires 2^64 activations — not practically reachable. No action needed.

Finding 2 — P2: parseMirror accepts epoch 0 as valid.

mode.go:355-361: parseMirror("incr:0") returns (true, 0, true). A mirror with epoch 0 would match a belief with epoch 0, which is what Activate produces when locked.Epoch was 0 (initial state). This is correct behavior — the first activation produces epoch 1, but if the initial row has epoch 0 and Activate bumps to 1, the mirror would be incr:1. A hand-written incr:0 would only match if the authority actually had epoch 0 and was activated, which would require the initial row to have mode=1, epoch=0 — an inconsistent state that Activate would not produce (it returns flipped=false for an already-activated row). Not a bug, but worth noting that epoch 0 is a valid mirror value.

Finding 3 — P2: assertExpectedMode error messages could leak deployment details.

mode.go:305-318: Error messages include the env var name and expected values. In a public-facing error, this reveals the deployment guard mechanism. Low risk — these errors are returned to the caller (internal allocation path), not directly to end users.

Finding 4 — P2 (borderline P1, but pre-existing design choice): Residual window when both Redis and DB evidence are lost.

As documented extensively in mode.go:68-76 and seq.go:743-746, if both the Redis counter key and the DB authority row are lost simultaneously (correlated Redis flush + DB restore), a cold process has no evidence of prior activation and delegates to GenSeq. The OCTO_BOTEVENT_EXPECTED_MODE=incr guard closes this, but only after it is armed. This is a documented residual, not a new defect. The design explicitly accepts this window during the flip itself.

Finding 5 — P2: refreshAuthority treats ErrStateMissing the same as "authority says legacy".

mode.go:457-462: Both err == nil (with !st.Activated()) and errors.Is(err, ErrStateMissing) fall into the same branch. This is correct for pre-migration deploys (the table doesn't exist yet = legacy), but it means a dropped table post-activation would also read as legacy. The positive-belief-is-terminal rule protects running processes, and legacyDelegate's counterExists check protects cold processes. The residual is the same as Finding 4.

Finding 6 — P2: install function always returns nil error.

mode.go:516-525: The install function signature returns (modeResolution, error) but never returns a non-nil error. This is dead API surface. The callers (refreshAuthority) check the error, which is always nil, so no harm — but the signature suggests an error path that does not exist.

Finding 7 — P2: belief.confirmed field is set but never read.

mode.go:102: The confirmed field distinguishes "authority said legacy" from "authority was unreadable", and the comment says "the flag exists so the log says which one happened." However, a search of the codebase shows confirmed is set in refreshAuthority (lines 455, 490) but never read by any function. It appears to be vestigial — the logging distinction was either removed or never implemented. Not a bug, but dead code.

Finding 8 — P2: noteMirrorRepairFailed copies the belief struct, including repairFailedFor from a prior failure.

mode.go:547-551: updated := *cur copies the entire belief, including any existing repairFailedFor and repairFailedAt. Then it overwrites repairFailedFor and repairFailedAt. This is correct behavior — the copy-on-write pattern replaces the old cooldown with the new one. No bug.

---

Summary

Both files are exceptionally well-engineered. The design has clearly been through multiple review iterations (referenced as P1-1 through P1-4, P1-A through P1-C throughout the comments). The asymmetric belief caching, epoch-stamped mirror values, and multi-layered evidence system (DB authority + Redis mirror + counter existence + process-local lastIssued) form a robust activation gate.

No P0 or P1 findings. The system correctly handles:

• Partial activation failures (self-healing via mirror rebuild)
• Redis data loss (RDB rollback detection + durable high-water marks)
• DB regression (positive belief terminal + counter existence check)
• Concurrent access (atomic pointers, mutex-guarded refresh, CAS for high-water marks)
• Stale mirrors (epoch comparison in Lua gate script)

The findings are all P2-level: dead code (confirmed field), minor API surface (install always returns nil error), and documented residual risks that require correlated multi-system failure to trigger.

[Octo-Q] verdict: APPROVE

The two files under review (mode.go and state.go) contain no blocking defects. The activation gate design is sound, concurrency handling is correct, and error paths are well-documented. All findings are P2 or below.

[lml2468]

PR Review (re-review) — PR #702 (Mininglamp-OSS/octo-server) — APPROVE (re-affirmed; owning a miss)

Re-reviewed at new head 2b4aac06b8c1 (forward from my APPROVEd dacb706b; commits refuse to activate while an unauthorized mirror is present, bound payload.robot_id syntactically before the existence lookup, record the fifth round). go build/go vet (botevent, robot, tools) exit 0 ✅; pure guard + mode/mirror + robot_id suites PASS; DB/Redis suites → CI.

Owning a miss — the negative-belief cache could swallow a real activation (Jerry-Xin / Steve 🔴)

At dacb706b I re-affirmed the mode machine as "verified across prior rounds" and did not re-scrutinise the negative-belief cache. It had a deniedMirror == mirror shortcut: a forged incr:1 denied at epoch 0 was cached by that exact string for the full negativeBeliefTTL (5s), and because Activate bumps epoch 0→1 and the tool then writes the same incr:1, the replica holding the cached denial would keep issuing GenSeq ids for up to 5s while other replicas were already on the counter — two id sources on one queue, the exact loss this PR exists to remove. Subtle (it needs the denied string to equal the activation string), but real, and I read past it. Jerry-Xin and Steve caught it. Owned.

The fix — delete the forged mirror instead of caching the denial (verified)

resolveMode no longer keys a cached denial on the mirror string. The deniedMirror field is gone; a mirror that claims incr now always falls through to a fresh authority read. When the authority denies a claiming mirror, the resolution carries unauthorizedMirror and the caller compare-and-deletes it (dropUnauthorizedMirror → dropMirrorScript deletes ModeKey only if it still holds the exact denied value — so a generation the operator wrote after the read, e.g. incr:2, is preserved). Deleting restores the invariant "absent mirror ⇒ consult the authority," so the next allocation re-reads and picks up a genuine activation immediately — no string-keyed swallow. TestUnauthorizedMirrorIsRemovedNotCached pins it.

The only residual cooldown is tightly scoped and safe: repairCooldown (1s) via repairFailedFor is set only when the compare-and-delete itself fails (noteMirrorRepairFailed, copy-on-write), i.e. Redis is rejecting writes — a state in which the counter's own INCR is failing too, so no allocation is succeeding on the counter to create a split; and it's 1s, not 5s, after which the authority is re-read. noteMirrorRepairFailed is a no-op on an activated/absent belief. The operator tool also now refuses to activate while an unauthorized mirror is present (e724140), closing the flip-time interleaving from the operator side.

yujiawei's remaining items closed

payload.robot_id is now bounded syntactically (plausible-id shape + column-length check) before the existence lookup, so a malformed id is rejected cheaply rather than costing a Redis/DB probe — TestPlausibleRobotID + TestRobotIDMaxLenMatchesTheColumn. The existRobot-error fail-open (don't drop events on a lookup error) from the prior round stays. Score-source guard breadth is explicitly marked deferred to #704 in the brief (not silently dropped).

Everything verified across prior rounds stands: authority-decides mode (mirror carries epoch, TestMirrorAloneCannotActivateTheCounter), positive-belief-terminal (TestNeverDowngrades…), cached-negative-no-per-alloc-DB, malformed-expected-mode fail-closed, the mode-key collision fix, the durable-high-water fail-closed bound (measures from last successful write, refuses past seedSafetyMargin), rollback detection, all-producers-on-NextEventID + repo-wide GenSeq guard, score==event_id, queue-key consolidation.

Verdict

APPROVE (re-affirmed at 2b4aac06) — the epoch-string-reuse swallow I missed at dacb706b is fixed at the root: a denied mirror is compare-and-deleted (preserving any newer generation) rather than cached by string, so a claiming mirror always re-reads the authority and a genuine activation is picked up with no mixed-source window; the only 1s cooldown applies solely while Redis is rejecting the delete (when the counter is unusable anyway), and the activation tool refuses to flip past an unauthorized mirror. payload.robot_id is bounded before the lookup, and every prior-round fix (fail-closed high-water bound, DB-authoritative mode, queue-key consolidation, rollback detection) remains intact — all test-locked. Both gates pass; merge stays behaviour-neutral (state seeds to legacy; a bare mirror can't flip it and is now actively removed). Thanks to Jerry-Xin and Steve — the denied-string == activation-string case was a real swallow I didn't trace.
Suggested decision: Safe to merge (gated, neutral) once CI runs the DB+Redis mode/mirror/high-water-bound/rollback suites green and the octo_bot_event_seq_state migration is applied. At activation: with no unauthorized mirror present, flip via the tool; watch AuthorityReads/MirrorUnauthorized/RollbacksDetected/highWaterWriteFailures. merge: MERGEABLE.

[Jerry-Xin]

The PR is relevant to octo-server and addresses a real bot-event queue allocator defect. I did not find a merge-blocking correctness issue in the allocator, activation gate, producer migration, or guards.

💬 Non-blocking

🟡 Warning — botevent-seq activate is not actually idempotent when the authority is already active and the mirror exists. activate() calls refuseUnauthorizedMirror() before botevent.Activate() can return flipped=false (tools/botevent-seq/main.go), and refuseUnauthorizedMirror() fatals on any valid mirror claim without checking the DB state (tools/botevent-seq/main.go). That makes the “already activated; reconcile mirror” branch mostly unreachable when the mirror is already correct (tools/botevent-seq/main.go). This is operational friction, not allocator corruption, but it should be fixed with a small state check and a tool test.

🔵 Suggestion — The “merge is behavior-neutral” claim is materially true for Redis-backed queue writers, but pre-activation allocation still performs a Redis probe before delegating to GenSeq (pkg/botevent/seq.go). For inline queries, addInlineQuery() now depends on that path before writing the in-memory event map (modules/robot/api.go). Since bot polling also reads Redis, I do not see this as a blocker, but the rollout language should avoid “exactly as before” if it means “no extra dependency touch.”

✅ Highlights

The core gate design is much stronger than the previous GenSeq shape: DB authority, generation-stamped Redis mirror, exact mirror comparison in the Lua gate, and no GenSeq fallback after activation are all aligned with the failure mode.

Producer coverage looks complete: the robot, group, bot API, botfather cleanup, and bot mention paths route through NextEventID/QueueKey, with source guards for GenSeq reintroduction and unrung queue writers.

I ran targeted checks:
go test ./pkg/botevent -run 'Test(NoGenSeq|GuardWould|Plausible|CounterKey)' -count=1
go test ./tools/botevent-seq -count=1
go test ./modules/robot -run 'TestPlausibleRobotID|TestRobotIDMaxLenMatchesTheColumn' -count=1
go test ./pkg/redis -count=1
git diff --check

modules/bot_api and modules/bot_mention integration tests could not run locally because MySQL on 127.0.0.1:3306 is unavailable; I did not count that as a PR failure.

Superseded: re-posting with repo-relative paths (dismissing to remove accidental local absolute paths from the body).

[Jerry-Xin]

The PR is relevant to octo-server and addresses a real bot-event queue allocator defect. I did not find a merge-blocking correctness issue in the allocator, activation gate, producer migration, or guards.

💬 Non-blocking

🟡 Warning — botevent-seq activate is not actually idempotent when the authority is already active and the mirror exists. activate() calls refuseUnauthorizedMirror() before botevent.Activate() can return flipped=false (tools/botevent-seq/main.go:269), and refuseUnauthorizedMirror() fatals on any valid mirror claim without checking the DB state (tools/botevent-seq/main.go:323). That makes the "already activated; reconcile mirror" branch mostly unreachable when the mirror is already correct (tools/botevent-seq/main.go:276). This is operational friction, not allocator corruption, but it should be fixed with a small state check and a tool test.

🔵 Suggestion — The "merge is behavior-neutral" claim is materially true for Redis-backed queue writers, but pre-activation allocation still performs a Redis probe before delegating to GenSeq (pkg/botevent/seq.go:596). For inline queries, addInlineQuery() now depends on that path before writing the in-memory event map (modules/robot/api.go:1020). Since bot polling also reads Redis, I do not see this as a blocker, but the rollout language should avoid "exactly as before" if it means "no extra dependency touch."

✅ Highlights

The core gate design is much stronger than the previous GenSeq shape: DB authority, generation-stamped Redis mirror, exact mirror comparison in the Lua gate, and no GenSeq fallback after activation are all aligned with the failure mode.

Producer coverage looks complete: the robot, group, bot API, botfather cleanup, and bot mention paths route through NextEventID/QueueKey, with source guards for GenSeq reintroduction and unrung queue writers.

Round-6 verification of the prior REQUEST_CHANGES items (byte-checked against the delta dacb706..2b4aac0):

• The mixed-source activation window I flagged is genuinely closed. The general negative-belief denial cache (deniedMirror) is removed entirely; a mirror claiming activation now always forces a fresh authority read (no TTL shortcut), and an authority-denied mirror is deleted via compare-and-delete on the exact value rather than cached. Activate bumping epoch 0→1 and writing incr:1 can no longer be swallowed. The only residual cache (repairFailedFor/repairCooldown = 1s) applies solely when the DEL itself failed — a state in which the counter's own INCR would be failing too, so no competing live id source exists. TestUnauthorizedMirrorIsRemovedNotCached pins exactly this: N allocations against a forged mirror issue one authority read, the key is removed, and a real activation to the same epoch takes effect on the next allocation with no swallow. This flips my previous 🔴 to resolved.
• The ZADD score-source guards (TestEveryBotEventQueueWriterRingsTheDoorbell, TestNoGenSeqForBotEventIDs) are present and match on the key symbol, and the doc/comment deviations are corrected to describe the delete-based mechanism and the #704-owned high-water bound.

I ran targeted checks:
go test ./pkg/botevent -run 'Test(NoGenSeq|GuardWould|Plausible|CounterKey)' -count=1
go test ./tools/botevent-seq -count=1
go test ./modules/robot -run 'TestPlausibleRobotID|TestRobotIDMaxLenMatchesTheColumn' -count=1
go test ./pkg/redis -count=1
git diff --check

modules/bot_api and modules/bot_mention integration tests could not run locally because MySQL on 127.0.0.1:3306 is unavailable; I did not count that as a PR failure.

[yujiawei]

Code Review — PR #702 (octo-server)

Reviewed at head 2b4aac06b8c1c8711a0e57e2511162041eb92e2f, merge base 40627cc0, 25 files / +5236 −90.
This is the seventh formal round (earlier heads: d34bdfb0, f7829596, 37f45fbd, 2f7e58da,
e90540e2, dacb706b). I read the incremental diff dacb706b..2b4aac06 against the findings that
blocked the last round, then re-read the allocator, the mode machine and the operator tool.

What is different about this round: the integration suite was actually executed. Every previous
round — mine included — reported "DB/Redis suites → CI" and reviewed the allocator by reading. I
brought up MySQL 8.0 + Redis 7 and ran it the way CI does. Full results in §6; the short version is
that pkg/botevent passes 49 tests, 0 skipped, under -race -shuffle=on -count=1, as do
pkg/redis, modules/robot, modules/bot_api and modules/message. That closes the largest blind
spot this PR has carried for six rounds, and it is how the finding below was confirmed rather than
argued.

Real progress this round: the durable-mark bound is out of the PR and onto #704 in one piece with the
arithmetic — that was the right call and it took a three-times-rewritten mechanism out of this
change; the denied-mirror cache is gone; invalidateSeeded clears seeded last; the staleness
shortcut re-checks the mirror claim; payload.robot_id is bounded syntactically; and the score-source
guard is explicitly deferred in the brief
(.octospec/tasks/bot-event-score-monotonic/brief.md:166) and recorded as #704 Gap 4. All four
documentation deviations from the last round are corrected.

The one blocking finding is in the mirror-repair mechanism that was rewritten this round, and the
fix is one condition at a call site that already holds the evidence it needs.

---

1. Spec compliance

Spec: ❌ — nothing missing and nothing over-built; two deviations where load-bearing prose no
longer describes the code.

Missing — nothing. The score-source guard that was neither built nor moved last round is now
marked DEFERRED in the brief and tracked as #704 Gap 4.
grep -rn '"robotEvent:' --include=*.go . is empty outside tests except the single QueueKeyPrefix
definition (pkg/botevent/seq.go:245).

Over-built — nothing. Every change in dacb706b..2b4aac06 maps to a finding from the last round.

Deviation 1 — "merging is behaviour-neutral … exactly as before" is not true of the I/O shape, and
this is the third round it has been raised. The Summary says "every replica delegates to GenSeq
exactly as before"; COMPREHENSION Q1 says "Until activation nothing changes at runtime"; Rollout step 1
says "every allocation goes through GenSeq with no DB round trip". The DB claim is true and
well tested (TestNotActivatedDoesNotQueryTheAuthorityPerAllocation). But pre-activation every
allocation now performs a Redis EVALSHA (probeAllocatorState, pkg/botevent/seq.go:596 and
:749) on the hottest producer path inside a held msgSem slot, where GenSeq served 999 of every
1000 allocations from a process-local block with no I/O at all. The disclosure does exist — but it is
in Rollout step 3, framed as a consequence of activation ("The I/O shape changed … every
allocation here is a Redis round trip"), when it actually starts at merge. addInlineQuery
(modules/robot/api.go:1020) is the sharpest case: it writes an in-memory map and previously needed no
Redis at all to obtain an id.

This is not a correctness problem — the queue ZADD on the same path already requires Redis, so
availability is unchanged for queue writers — but the merge-safety argument for this PR is
behaviour-neutrality, and the sentence stating it is the one that is wrong. Move the step-3 paragraph
up beside the claim, or qualify it as "same allocator, one added Redis round trip".

Deviation 2 — pkg/botevent/mode.go:174-178 asserts a benignness property the code does not have.
The comment on modeResolution.unauthorizedMirror reads: "The race is benign: if an operator
activates between the authority read and this delete, a legitimate mirror is removed, every replica's
next allocation consults the authority, finds it activated, and rebuilds it. One extra read each, no
mixed source." Both halves are wrong:

• "every replica's next allocation consults the authority" — a replica holding an unexpired negative
  belief that sees an absent mirror answers decideLegacy straight from cache without consulting
  the authority (pkg/botevent/mode.go:390-392; an absent mirror does not claim, so the conflict
  rule that forces a read never fires). Recovery on those replicas waits for negativeBeliefTTL, not
  for one allocation.
• "no mixed source" — see P1-1. When the authority is the party that regressed, deleting the mirror
  is not benign in any direction.

Everything else I re-checked matches. The four last-round deviations are corrected:
pkg/botevent/seq.go:936-979 now states plainly that the trail is unbounded and names #704; the
pointer to the deleted noteHighWaterFailure is gone; unpersistedSpan and its docstring went with
the mechanism; Rollout step 6 names three metrics and says which one does not self-heal. TestMain
runs the two real CREATE TABLEs (pkg/botevent/main_test.go:61-66). The 2^50 floor cap, the
CROSSSLOT constraint (pkg/botevent/seq.go:82-87) and the MySQL ≥ 8.0.16 CHECK assumption
(modules/robot/sql/20260805000001_bot_event_seq_state.sql:48-49) are all written down.

---

2. Code quality

Quality: Changes-Requested

P1-1 — a single cold replica deletes the legitimate activation mirror when the authority has regressed, turning a metadata-only regression into a fleet-wide bot-event outage

This is the mechanism that replaced the denied-mirror cache in 0948ace2, and it is an ordering defect
of the same family as the P1-3 accepted in round 4 ("the global mirror was published before the per-bot
seed"): the evidence that invalidates the delete's premise is computed before the delete and
consulted after it.

allocate has counterExists in hand at pkg/botevent/seq.go:596. It deletes the mirror at
:604-608. Only at :611 does it pass that same counterExists to legacyDelegate, which uses it to
conclude "the authority was activated and has regressed" and refuse (:727-743, with
counterFoundWithoutAuthority.inc() at :734). So the code establishes that the authority is the
unreliable party — after having destroyed the mirror on the authority's word.

Reproduced at this head against real MySQL 8.0 + Redis 7:

authority mode=1 epoch=0, mirror "incr:0", counter present, one id allocated
  → authority regresses to mode=0   (restore of a pre-activation dump, or a manual UPDATE)
  → Redis keeps everything:          mirror present, counter present
  → ONE freshly started replica allocates once:
       warn  "mode mirror claims activation but the authority does not; removing the stale mirror"
       error "...already has a counter — ... the authority was activated and has regressed.
              Refusing to issue a legacy id below the counter's range."   ← correct, fail-closed
       GET botEventSeq:mode  →  redis.Nil          ← the LEGITIMATE mirror is gone
  → a still-healthy activated replica's gate:
       runGate(client, robotID, MirrorValue(0)) = -1     (gateNotActivated)

-1 sends that replica into gateClosed (pkg/botevent/seq.go:692-702) →
refreshAuthority(..., force: true) → the authority says legacy while prior.activated → the
deliberate refusal at pkg/botevent/mode.go:464-474. Every enqueue for every bot on every activated
replica then fails, and it does not recover until the octo_bot_event_seq_state row is restored.

Contrast with the previous head: the mirror stayed, the cold replica refused itself, and healthy
replicas kept allocating correctly from the counter. So one new pod — a rolling restart, an HPA
scale-up, a crash-loop — now converts a bookkeeping loss with zero delivery impact into a total
bot-event delivery outage. That is not the fail-closed direction; it is discarding the last correct
artifact on the word of the artifact already known to be wrong.

Why the new test cannot see it: TestUnauthorizedMirrorIsRemovedNotCached
(pkg/botevent/seq_review_test.go:184-236) calls client.Del(SeqKey(robotID)) at :194, so
counterExists is false for the entire test. It exercises only forged-mirror-with-no-counter, where
deleting is exactly right. Third round in a row where the test shipped alongside a fix asserts the
neighbouring property.

Minimum fix — one condition, at pkg/botevent/seq.go:604:

// Do not "repair" the mirror on evidence that the *authority* is the regressed party:
// counterExists means some replica allocated from this counter, which can only have
// happened after the authority said incr.
if res.unauthorizedMirror != "" && !counterExists {
    dropUnauthorizedMirror(client, res.unauthorizedMirror)
}

Worth one step further, because !counterExists narrows the window rather than closing it (the mirror
is global, a seed is per-bot, so an allocation for a brand-new bot still has no counter to object
with): also suppress the delete when the authority read returned ErrStateMissing. A dropped table or
a missing row is not evidence that anybody forged a mirror, and pkg/botevent/state.go:69-70 already
says readers must distinguish "the migration has not run" from "the authority is wrong". Then a test
whose fixture keeps both the mirror and the counter while the authority regresses — that is the
only shape that catches this class.

P2

1. botevent-seq -action activate is not idempotent, and its refusal asserts a fact it never
   checked. Found independently by another reviewer on this PR; I confirmed it in code.
   refuseUnauthorizedMirror runs at tools/botevent-seq/main.go:269, before botevent.Activate
   can return flipped=false, and it fatals on any parseable mirror claim (:332-344) without ever
   reading the state row. So on an already-activated system with a correct mirror — the normal state
   after a successful flip — re-running activate -yes dies with "already holds incr:1 while the
   authority says legacy. Nothing should have written that — it means this Redis was activated
   before, is shared with another environment, or was restored from an activated snapshot. Confirm
   which, then DEL the key and rerun." Every clause is false in the common case, and it instructs the
   operator to delete the live mirror. The "already activated; reconcile the mirror" branch at
   :276-283 — the documented repair for a stale or missing mirror — is unreachable whenever a mirror
   is present. P2 rather than P1 because it is loud, exits non-zero, changes no state, and DELing the
   mirror against a healthy authority self-heals (the forced authority read finds incr and rebuilds
   it). Fix: read the state row first and refuse only when the authority really says legacy.
   tools/botevent-seq has no test file at all, and this is the second finding in it across two
   rounds.
2. The retained per-bot counter is an unbounded, user-triggerable growth surface, and it sits
   awkwardly beside the reasoning used to justify the payload.robot_id gate. Raised by an
   independent analysis pass; I confirmed the mechanism and narrowed the severity.
   modules/botfather/api_user.go:470-479 deliberately keeps botEventSeq:counter:{botID} (no TTL,
   noeviction) and seq:botEventHigh:{botID} when a bot is deleted, and the comment argues the trade
   well. What it does not say is that the loop create bot → have it receive one message → delete bot
   is available to any authenticated user and leaves both artifacts behind permanently, so growth is
   bounded by bot-creation rate rather than by live bot count. I am ranking this P2, not higher, for
   two reasons the raising pass did not have: the legacy seq:robotEventSeq:{botID} row already
   leaked identically before this PR, so only the small Redis key is new; and the artifacts are created
   by seedCounter/persistHighWater on first allocation, so a bot that never receives an event
   leaves nothing. Still worth a sentence in that comment — the PR bounds an arbitrary client string
   on exactly this reasoning two files away, and a reader deserves to see why bot-count scale is
   accepted where payload scale was not. A dated sweep that proves the id is unused is the stated exit;
   #704 is the natural home.
3. plausibleRobotID bounds bytes while its justification is characters.
   modules/robot/event.go:57 (len(candidate) > robotIDMaxLen) counts bytes; the docstring at
   :32-34 justifies the bound as the width of robot.robot_id VARCHAR(40), which MySQL counts in
   characters under utf8mb4. An independent pass rated this P1 on the grounds that a real bot with
   a multibyte id would have its events silently dropped. I adjudicate it down to P2, with the
   evidence: both id-producing paths are ASCII-only — a client-supplied username is restricted to
   [a-z0-9_]{1,20} plus _bot (modules/botfather/api_user.go:161-176, const.go:20), and the
   auto-generated form is lowercase hex plus _bot (modules/botfather/command.go:954-957). So no
   reachable robot_id is multibyte and no real bot's events can be dropped. It remains a nit worth
   fixing because the stated reason is not what the code enforces: either say "bytes, deliberately
   stricter than the column" or use utf8.RuneCountInString.
4. belief.confirmed is write-only. Set at pkg/botevent/mode.go:459 and :491, read nowhere.
   Its own docstring (:129-132) says "the flag exists so the log says which one happened" — no log
   reads it. Same documentation-versus-code class this PR has been closing each round: log it or delete
   it.
5. install never returns a non-nil error (pkg/botevent/mode.go:522-531) while all three call
   sites check one. Dead API surface implying a failure path that does not exist.
6. invalidateSeeded's docstring now overstates why its order matters
   (pkg/botevent/seq.go:795-801). The fix is right and I asked for it. But with the span bound gone,
   lastPersisted is purely a throttle marker, so the worst outcome of the bad interleaving is one
   redundant INSERT — not "a bot treated as seeded whose bookkeeping this process no longer has".
7. Un-stated id-space burn (carried, still unwritten). seedCounter computes
   floor = max(queueMax, legacyMax, durableMax, stateFloor) + seedSafetyMargin
   (pkg/botevent/seq.go:1100-1118) on each process's first allocation per bot, so every process
   restart raises every active bot's counter by ~2000. Harmless for delivery (the cursor is exclusive
   and tolerates gaps) and irrelevant for int64, but it compounds with restarts and interacts with the
   2^50 cap the operator tool enforces.
8. counterExists is EXISTS and nothing more (pkg/botevent/seq.go:749-770): no provenance, no
   TTL, nothing ever removes the key — and P1-1 makes it load-bearing in a second place. Belongs with
   the rest of the rollback-recovery design on #704, which does not currently list it.

Raised elsewhere and adjudicated against

• "The pre-activation Redis probe is blocking." No. The queue ZADD on the same path already
  requires Redis, so availability is unchanged for queue writers; addInlineQuery is the only genuinely
  new Redis dependency, and it is one call. It is a description problem (Deviation 1), not a
  correctness one.
• "The activate non-idempotency is allocator corruption." No — see P2-1: no state changes, and the
  mis-instructed DEL self-heals against a healthy authority.
• "The byte-length bound silently drops real bots' events (P1)." Not reachable — see P2-3 for the
  two ASCII-only id-producing paths.

---

3. Verdict

CHANGES_REQUESTED — Spec: ❌ and Quality: Changes-Requested; either one blocks.

Being precise about what I am not saying, since three other reviewers have approved this head and I
am the only remaining block. The diagnosis, the design, the activation gate and the producer
consolidation are right, and I am asking for none of them to change. The merged state is genuinely
inert, and this round I verified that by execution rather than by reading. P1-1 is reachable only
after activation, which #704 gates.

What decides it: P1-1 is a confirmed, reproducible, fleet-wide-outage regression introduced by this
head, and it is currently neither fixed nor tracked — #704's Gaps 1–4 say nothing about mirror repair.
Merging as-is loses it. Unlike the durable-mark bound, it is not a design question: counterExists is
already computed four lines above the delete, so this is a one-condition change, not a fourth attempt
at a mechanism. Either land that condition plus a test whose fixture keeps the mirror and the
counter, or add it to #704 as a fifth gap — either flips me to approve, and I do not care which.

---

4. Suggested split

Fix here:

1. Do not delete the mirror when counterExists — and preferably not on ErrStateMissing either —
   plus a test with both artifacts surviving (P1-1).
2. Read the state row in refuseUnauthorizedMirror before refusing, and fix the message that asserts
   "while the authority says legacy". A first test file for tools/botevent-seq while you are there
   (P2-1).
3. Move the Rollout step-3 I/O paragraph up beside the behaviour-neutrality claim, and correct the
   "race is benign" comment at mode.go:174-178 (Deviations 1 and 2).
4. Four one-liners: byte-versus-character bound, the write-only confirmed field, install's error
   return, the invalidateSeeded docstring (P2-3 … P2-6).
5. One sentence in modules/botfather/api_user.go:470-479 on why bot-count-scale retained state is
   accepted where payload-scale was not (P2-2).

Defer to #704, alongside the durable-mark bound that correctly landed there this round: the id-space
burn note, counterExists provenance, and the reclaiming sweep for retained counters (P2-7, P2-8,
P2-2).

---

5. Process note — seventh round

Rounds 5 and 6 each found that the previous round's fix to the same mechanism was wrong in a new way.
Round 7 does too — but the mechanism moved. The durable-mark bound was rewritten three times and is now
on #704 in one piece with the arithmetic showing why a local patch cannot fix it. That was the right
call and it took the recurring failure out of this PR. The recurrence has simply relocated to
mirror repair, which has now been rewritten twice (cache the denial → delete the key), and the second
attempt has a new failure mode.

So the recommendation is narrower than last round's, not broader: one more targeted round, not
another redesign. P1-1 is a one-line guard whose evidence is already in scope; P2-1 is a missing
state read in a tool with no tests. If a fix to either lands wrong again, stop patching and escalate to
whoever owns the change: at that point the pattern says this file is not verifiable by review alone,
and the answer is a call on what else moves behind the activation gate — not an eighth round.

Two things for whoever owns the merge decision:

• tools/botevent-seq carries the entire activation procedure and has zero tests, while being the
  only component a human runs by hand at the cutover. Two findings in it across two rounds.
• The needs-human-review item is unchanged and still not this PR's job: payload.robot_id
  (modules/robot/event.go:206-249) is checked for bot existence, not authorization, while the
  sibling DM branch in the same function checks creator and friendship. It stays listed because this PR
  touches that line.

---

6. Coverage and blind spots

Stated so the gaps are not read as clearance.

Executed at this head — MySQL 8.0 + Redis 7 in throwaway containers, per-package database drop and
recreate exactly as .github/workflows/ci.yml:276 does, with OCTO_MASTER_KEY set as CI sets it:

package	result
pkg/botevent -race -shuffle=on -count=1	PASS — 49 tests, 0 skipped, 7.4 s
pkg/redis -race	PASS
modules/robot -race -shuffle=on	PASS (needs OCTO_MASTER_KEY; panics without it)
modules/bot_api -race -shuffle=on	PASS, 62.9 s
modules/message -race -shuffle=on	PASS
go build ./... / go vet (botevent, robot, tools)	clean

The "0 skipped" claim is therefore verified rather than trusted, which matters because a t.Skipf
here is indistinguishable from a pass and that is precisely what an earlier revision shipped.
TestFailedDurableWriteIsThrottledAndDoesNotFailTheEnqueue does assert the residual is still
present — it logged "3000 ids issued past a frozen durable mark at 1; a seed recovering from that
mark alone would land at 2001, below ids already issued" — so it will serve as #704's regression test
as advertised. The octo_bot_event_seq_state migration applied through real sql-migrate as part of the
modules/robot run.

• Not run by me: modules/group and modules/botfather. The author reports pre-existing failures
  at both this head and 2f7e58da; repository CI is green at this head (Test, Build, Vet, Lint
  all SUCCESS), so CI is the authority there.
• go test ./... in one shot still does not work in this repo, and pkg/botevent's TestMain
  gives it a new shape: it creates the migration-owned seq table in the shared test schema, so a
  later package's migration aborts with Error 1050: Table 'seq' already exists instead of the
  pre-existing unknown migration in database. I checked before counting this — ci.yml:255-262
  already documents go test ./... as broken and drops the database between packages — so it is a
  pre-existing repository condition, not a finding against this PR. Worth knowing if you run the
  suite locally.
• Independent analysis passes: two ran and both returned. One is degraded and says so itself — its
  input was truncated before the allocator, so it never saw NextEventID, resolveMode, or the Lua
  scripts, and its silence on those is absence, not agreement; what it did reach is credited at
  P2-2 and P2-3. The other reviewers on this PR reached the pre-activation-probe point independently
  (Deviation 1) and the activate non-idempotency (P2-1), which is why both are credited rather than
  claimed. Nothing here rests on a pass that failed or timed out; where a pass disagreed on severity I
  said so and gave the evidence (P2-3).
• Not examined by anyone this round: tools/genseq-repro/main.go; the consumer-side cursor code
  beyond the docstring rewrite at modules/bot_api/events.go:175-218; and Redis namespace isolation
  between environments, which P1-1, P2-1 and P2-8 all depend on and which is a deployment fact, not a
  code fact.
• What execution cannot cover: the three preconditions botevent-seq demands -yes for (#704
  closed, no pre-fix replica remaining, a brief write pause) are operational, and the co-recovery
  argument rests on production running appendonly no / maxmemory 0 / noeviction in the same
  instance and db as the queue. Both remain human-verified facts — appropriate for the
  needs-human-review label, and pkg/botevent/seq.go:82-87 is the right place to have written the
  second one down.

[an9xyz]

Round 7 addressed at 23757086. @yujiawei — P1-1 is confirmed and fixed, and I reproduced it
before touching any code, which is the thing the last three rounds got wrong.

The reproduction (TestRegressedAuthorityDoesNotDestroyTheLegitimateMirror) was written
first and watched failing, with exactly the shape you described:

--- FAIL: TestRegressedAuthorityDoesNotDestroyTheLegitimateMirror
    the legitimate mirror "incr:0" was destroyed (redis: nil). One cold replica has now
    taken every activated replica's gate down …

The fix is not the !counterExists guard you offered as the minimum. You already named
why: the mirror is global while a counter is per-bot, so an allocation for a bot that has
never received an event still has no counter to object with — it narrows the window rather
than closing it. And the delete's only benefit was suppressing the authority-read storm,
which the denial cooldown also suppresses. So the delete is gone entirely rather than
conditioned: the allocator never writes the mirror on the legacy path now. Only the
operator tool does, which is the one place a human sees what they are overwriting.

That means the mechanism has now been through cache → delete → neither, and I want to be
straight about what the third one costs rather than present it as free:

• The cooldown is back: 1s, keyed on the exact denied value. A genuine activation writing
  byte-identical bytes to a previously denied mirror is not noticed for up to that second.
• That window cannot be zero without reading the authority per allocation, because
  nothing visible from Redis distinguishes a forged incr:1 from the real one. Only the
  authority does. @Jerry-Xin — this is a partial walk-back of what you credited last round
  ("always forces a fresh authority read, no TTL shortcut"), and I would rather say so than
  let it read as unchanged.
• It is only entered when a mirror claiming activation already sits against a legacy
  authority, which botevent-seq -action activate now refuses to flip on top of.
• It is armed only when no counter exists. With one, the authority is the regressed
  party, every allocation for that bot is refused anyway, and suppressing the read would
  only delay noticing recovery.

If either of you thinks 1s-with-a-supervised-precondition is the wrong side of that trade,
say so and I will take the read storm instead — it is a one-line change either way, and the
storm at least fails in the direction of noticing things.

P2-1 — fixed by reading the state row first, and the tool has its first test file. You
were right that this was the more damning half: it carries the whole activation procedure, is
the only component run by hand at the cutover, and had produced two findings across two rounds
with zero tests. judgeMirror is split out so the seven-case matrix runs without Redis or
MySQL; there is a test pinning that the refusal is the only message allowed to say "DEL",
since instructing a DEL of a live mirror was the concrete harm.

Both deviations — the behaviour-neutrality claim is fixed in the claim, in Summary,
beside the sentence it qualifies, with addInlineQuery named as the only genuinely new Redis
dependency. Third round you raised it; it should not have taken three. The "race is benign"
comment went with the mechanism it described.

P2-3 … P2-6 all done as one-liners, and on the byte-vs-character bound I took your
adjudication: the docstring now says the gate is deliberately the stricter of the two and
records the evidence that it is unreachable today, rather than justifying itself with a column
width it does not match.

#704 gained Gap 5 — mirror repair has no safe automatic form, both attempts and why each
failed, so nobody tries a third. Plus counterExists provenance, the id-space burn on restart,
and the reclaiming sweep, all of which you asked to move there.

Verification

Ran the way you did, per-package DB reset, OCTO_MASTER_KEY set:

package
pkg/botevent -race -shuffle=on	46 tests, 0 skipped
tools/botevent-seq (new)	ok
pkg/redis, modules/robot, modules/bot_api, modules/message	ok

go build ./..., go vet ./..., golangci-lint on the touched packages, both i18n gates,
git diff --check clean.

One local diagnosis worth recording so it is not re-derived: modules/message's
TestE2E_Issue557_* went red consistently this round, not intermittently, and it was the
local WuKongIM container's raft (propose batch until applied timeout /
store message failed: context deadline exceeded). Identical failure at 2b4aac06 in a clean
worktree; green after restarting the container. Not this PR.

On your process note: agreed, and this is the round where I followed it exactly — reproduce
first, one targeted change, and the escalation condition is live. If the mirror decision above
is wrong again, it is not getting a fourth mechanism from me; it goes to #704 with the
durable-mark bound and the activation gate holds the line.

[Jerry-Xin]

This PR is in scope for octo-server and addresses bot event delivery state, allocation, queue producers, and rollout tooling. I did not find any blocking correctness, security, or architecture issues.

Non-blocking

🟡 Warning: pkg/botevent/mode.go logs “removing the stale mirror”, but the current allocator deliberately no longer deletes or repairs that mirror on the legacy path. The behavior is correct per the surrounding design, but the log message is now misleading during exactly the incident path operators will inspect. I would change it to “ignoring the stale mirror” or “denying the mirror claim”.

Highlights

✅ The relevance gate passes: the PR modifies bot event id allocation and delivery behavior that belongs directly to this repository.

✅ The critical queue writers I checked route ids through botevent.NextEventID, and queue key spelling is consolidated through botevent.QueueKey.

✅ The activation gate design is materially safer than a Redis-only switch: MySQL remains authoritative, the Redis mirror is generation-stamped, and the tool refuses unsafe activation states.

✅ The tests are unusually strong for this risk level: source guards, rollback/mirror cases, residual-loss pinning, and operator-tool tests are all present.

Verification Performed

I ran:

go test ./pkg/botevent ./tools/botevent-seq
go test ./tools/genseq-repro
go test ./modules/robot -run 'TestPlausibleRobotID|TestRobotIDMaxLenMatchesTheColumn'
go test ./modules/bot_api ./modules/group ./modules/botfather -run '^$'
go test ./modules/robot -run '^$'
git diff --check

All passed in this checkout.

Superseded: re-posting with repository-relative paths.

[Jerry-Xin]

This PR is in scope for octo-server and addresses bot event delivery state, allocation, queue producers, and rollout tooling. I did not find any blocking correctness, security, or architecture issues.

Non-blocking

🟡 Warning: pkg/botevent/mode.go:490 logs “removing the stale mirror”, but the current allocator deliberately no longer deletes or repairs that mirror on the legacy path (the dropUnauthorizedMirror mechanism was removed this head; the path now only records the denial in-memory via noteMirrorUnauthorized). The behavior is correct per the surrounding design, but the log message is now misleading during exactly the incident path operators will inspect. I would change it to “ignoring the stale mirror” or “denying the mirror claim”.

Round 7 CR items (re-verified byte-for-byte against 2375708 vs the prior head 2b4aac0)

✅ Mirror-repair call-condition (P1-1) — resolved. The whole repair mechanism (dropUnauthorizedMirror / dropMirrorScript / noteMirrorRepairFailed) is deleted, with zero remaining references. The allocator no longer writes or deletes the mode mirror on any path; it only records the denial in-memory (noteMirrorUnauthorized), and only when !counterExists. The fleet-wide-outage case (a cold replica deleting the mirror every healthy activated replica's gate compares against) is now structurally impossible.

✅ Merge-safety I/O-shape wording — corrected: the added pre-activation Redis round trip is now stated up front, with addInlineQuery named as the only genuinely new dependency, and pinned by TestNotActivatedDoesNotQueryTheAuthorityPerAllocation (no per-allocation DB read).

✅ Code/doc consistency — the write-only confirmed field and install's never-returned error are removed, and docstrings corrected; the one residual is the stale log string above (non-blocking).

Highlights

✅ The relevance gate passes: the PR modifies bot event id allocation and delivery behavior that belongs directly to this repository.

✅ The critical queue writers route ids through botevent.NextEventID, and queue key spelling is consolidated through botevent.QueueKey.

✅ The activation gate design is materially safer than a Redis-only switch: MySQL remains authoritative, the Redis mirror is generation-stamped, and the tool now reads the authority before judging the mirror (judgeMirror) so a valid re-run of activate is idempotent.

✅ Tests are unusually strong for this risk level: source guards, rollback/mirror cases, residual-loss pinning, and the operator tool's first tests are all present.

Verification Performed

• Byte-isolated the true delta (git diff 2b4aac06 2375708): 2375708 is 4 commits ahead / 0 behind — genuine new work, not rebase drift.
• go build ./pkg/botevent/... ./tools/botevent-seq/... ./modules/robot/... — clean.
• go test ./pkg/botevent ./tools/botevent-seq ./tools/genseq-repro; modules/robot -run 'TestPlausibleRobotID|TestRobotIDMaxLenMatchesTheColumn'; git diff --check — all passed in this checkout.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Octo-Q automated review Review: octo-server PR#702 — botevent monotonic ID allocator

Reviewer: Octo-Q (automated review)
Head SHA: 2375708662d5e0b7650742e070e8df25f5ee727d
Diff scope: pr-base...HEAD (three new files: seq.go, mode.go, state.go)

---

1. Verification Summary

All three files are new additions (no prior version). I read every line of all three files (1987 lines total) and traced data flow through every allocation path, every state transition, and every error branch.

Overall assessment: This is an exceptionally well-engineered change. The documentation is unusually thorough — every design decision is justified inline with references to specific review rounds and production incidents. The code matches its documentation. I found no P1 defects.

---

2. Data-Flow Tracing

2.1 Allocation hot path (post-activation, seeded)

NextEventID(ctx, robotID)
  → nextEventID(ctx, SeqClient(cfg), robotID)
    → allocate(ctx, client, robotID, 0)
      → [fast path] activeBelief.Load() → b.activated=true, seeded has robotID
        → allocateFromCounter(ctx, client, robotID, b, 0)
          → runGate(client, robotID, b.mirrorValue())
            → Lua gateSource: GET(ModeKey) == expectMirror ? INCR(SeqKey) : sentinel
          → afterIssue(ctx, client, robotID, v, 0)
            → checkRegression(robotID, v) — CAS-safe against lastIssued
            → recordIssued(robotID, v) — monotonic CAS advance
            → persistHighWater(ctx, robotID, v) — best-effort, throttled

Verified: b.mirrorValue() returns "incr:{epoch}" from the belief struct. The Lua script atomically compares the live Redis ModeKey value against this exact string. A mismatch returns -1 (gateNotActivated), not an id. The INCR only fires when the mirror matches AND the counter key EXISTS. No data can be consumed without passing through this atomic gate.

2.2 Allocation cold path (pre-activation or first-use)

allocate() → probeAllocatorState(client, robotID)
  → Lua probeSource: returns {mode_mirror, counter_exists} in one RTT
  → resolveMode(ctx, mirror)
    → activeBelief nil or stale → refreshAuthority(ctx, claims, mirror, false)
      → readStateDeadlined(ctx) → ReadStateContext with 300ms deadline
        → SELECT mode, epoch, cutover_floor FROM octo_bot_event_seq_state WHERE singleton_id=1
      → install belief atomically
  → decideLegacy → legacyDelegate(ctx, robotID, counterExists)
    → refuse if lastIssued has entry (this process already issued counter ids)
    → refuse if counterExists (some process allocated from counter)
    → else → legacyEventID → ctx.GenSeq(...)

Verified: The counterExists flag comes from the same Lua script that reads the mirror — one atomic probe. The lastIssued check is process-local and cannot be stale in the dangerous direction (it only has entries after this process issued a counter id, which requires the gate to have opened, which requires the authority to have said incr).

2.3 Seed floor computation

seedCounter(ctx, client, robotID):
  floor = max(queueCeiling, legacyCeiling, highWaterCeiling, stateFloorOrZero)
  if floor > 0: floor += seedSafetyMargin (2000)
  → Lua seedSource: SET counter to floor only if current < floor

Upstream sources traced:

• queueCeiling: ZRevRangeWithScores(QueueKey, 0, 0) → top[0].Score — the highest score in the live queue. Returns 0 if queue is empty. Correct.
• legacyCeiling: SELECT min_seq FROM seq WHERE key=? — the GenSeq block boundary. Returns 0 if no row. Correct.
• highWaterCeiling: SELECT min_seq FROM seq WHERE key=? (high-water key) — the durable mark. Returns 0 if no row. Correct.
• stateFloorOrZero: ReadStateContext → st.CutoverFloor. Returns 0 on any error (best-effort backstop). Correct, with caveat — see P2-1.

2.4 Activation state transition

Activate(ctx, floor, observedMax):
  BEGIN TX
  SELECT mode, epoch FROM state WHERE singleton_id=1 FOR UPDATE
  → if already incr: return (false, epoch, nil) — idempotent
  → if floor <= observedMax: return ErrFloorTooLow
  → UPDATE state SET mode=1, epoch=epoch+1, cutover_floor=floor
     WHERE singleton_id=1 AND mode=0
  → check affected=1 (CAS guard)
  COMMIT

Verified: The FOR UPDATE lock serialises concurrent operators. The AND mode=0 in the UPDATE is a CAS guard — if another operator flipped between the SELECT and UPDATE, affected=0 and the function errors. The returned epoch (locked.Epoch+1) matches what was written. Correct.

---

3. Findings

P2-1: stateFloorOrZero swallows all errors including non-transient ones

File: pkg/botevent/state.go:199-208
Diff-scope: New code.
Description: stateFloorOrZero returns 0 on any error from ReadStateContext, including permanent errors like a schema mismatch or a corrupt row. The comment says "best-effort" and this is true for transient errors (DB timeout), but a permanent error (e.g., the state row exists but has a NULL cutover_floor column after a botched migration) would silently lose this floor source forever. Since this is the backstop of four floor sources and the other three are independent, the practical risk is low — but a logged warning on non-transient errors would aid diagnosis.
Fix direction: Distinguish ErrStateMissing (expected pre-migration) from other errors, and log the latter at warn level. The seed path already has a deadline so the latency impact is bounded.

P2-2: seedLocks sync.Map grows unboundedly — one mutex per bot, never cleaned

File: pkg/botevent/seq.go:912-918
Diff-scope: New code.
Description: seedMutex(robotID) creates a *sync.Mutex per distinct robotID via seedLocks.LoadOrStore. These entries are never deleted in production code (ResetSeededForTest clears them but that is test-only). In a long-running process serving many bots, this grows without bound. With UUID-hex robot IDs each entry is roughly 100 bytes (mutex + map overhead), so 100K distinct bots consume roughly 10MB. Not catastrophic, but unlike seeded / lastIssued / lastPersisted (which invalidateSeeded clears on mirror rebuild), seedLocks has no eviction path at all.
Fix direction: Acceptable as-is for the current deployment scale. If bot count grows, consider either (a) including seedLocks in invalidateSeeded (safe — the next reseed re-creates the mutex via LoadOrStore), or (b) using a sharded lock pool (e.g., 256 mutexes keyed by hash(robotID) % 256) to cap memory.

P2-3: afterIssue regression recovery does not distinguish gateCounterMissing from a true regression

File: pkg/botevent/seq.go:855-862
Diff-scope: New code.
Description: In afterIssue, after regression detection and re-seed, the code runs runGate again and checks:

if retried == gateNotActivated {
    return gateClosed(ctx, client, robotID, attempt+1)
}
if retried <= prev {
    return 0, fmt.Errorf("...re-seeding only reached %d...", retried)
}

If the gate returns gateCounterMissing (-2) — meaning the mirror matches but the counter was just deleted again — the value -2 falls through to retried <= prev (since prev >= 0), producing the misleading error "re-seeding only reached -2." The refusal itself is correct (safe behaviour), but the error message would confuse an incident responder.
Fix direction: Add an explicit case gateCounterMissing check before the <= prev comparison, returning a more descriptive error like "counter disappeared again during regression recovery."

P2-4: persistHighWater trails without bound while DB writes fail (documented residual)

File: pkg/botevent/seq.go:940-991
Diff-scope: New code.
Description: While durable writes are failing, lastPersisted advances on failure (re-arming the throttle) but the actual MySQL mark does not. The gap between issued ids and the durable mark grows without bound. A Redis rollback during a prolonged DB outage would then seed below live cursors. The code documents this explicitly and defers it to #704, which must be closed before activation.
Verdict: Known residual, gated by #704. Not a blocker for merge. The dmwork_bot_event_seq_high_water_write_failure_total metric provides the alert signal.

P2-5: highWaterCeiling and legacyCeiling discard the row count from dbr Load

File: pkg/botevent/seq.go:1124-1134, seq.go:1142-1153
Diff-scope: New code.
Description: Both functions discard the count returned by LoadContext:

if _, err := ctx.DB().Select("min_seq").From(...).Where(...).LoadContext(deadline, &mark); err != nil {
    return 0, ...
}
return mark, nil

When the row does not exist, dbr returns count=0, err=nil and mark stays at its zero value (0). This is the correct behaviour for "no ceiling recorded" — but the discarded count means a future reader cannot tell whether 0 means "no row" or "row exists with min_seq=0." In practice these are equivalent (both mean "start from scratch"), so this is a readability concern, not a correctness one.
Fix direction: Nit. Optionally capture count and explicitly return 0 when count==0 for clarity.

Nit-1: parseMirror accepts leading/trailing whitespace via TrimSpace

File: pkg/botevent/mode.go:297
Diff-scope: New code.
Description: parseMirror calls strings.TrimSpace(v) before parsing. This means a hand-written SET botEventSeq:mode " incr:5 " would be accepted as epoch 5. While this is defensive parsing rather than a vulnerability, it creates an asymmetry: formatMirror produces "incr:5" (no spaces), so a mirror value with spaces would never match a validated belief's mirrorValue(), causing the gate to close and forcing an authority read on every allocation until the key is corrected. Not a defect — the gate closes safely — but the TrimSpace is doing no useful work since the only writer (formatMirror / MirrorValue) never produces padded output.
Fix direction: Harmless as-is. If strictness is preferred, remove the TrimSpace and let padded values fall through as "not a claim."

Nit-2: invalidateSeeded clearing order is lastPersisted then seeded, but the comment documents the reverse

File: pkg/botevent/seq.go:800-811
Diff-scope: New code.
Description: The code iterates []*sync.Map{&lastPersisted, &seeded}, clearing lastPersisted first, then seeded. The comment above correctly explains why seeded should be cleared last ("a marker should not outlive the state it stands for"). The code matches the comment's intent. However, the comment also says "review P1-2" requested this order, and a reader might wonder why lastIssued is not in the list — it is deliberately excluded because clearing it would disable regression detection. Worth a one-line note in the comment.
Fix direction: Add a sentence: "lastIssued is deliberately not cleared here: it is the rollback detector's memory, and clearing it would blind the next allocation to a counter regression."

---

4. Security-Sensitive Checklist (C1-C6)

C1 — Dual-path parity (activate/deactivate symmetry):
The Activate function flips mode 0→1 with FOR UPDATE + CAS. There is no Deactivate function in this PR. The state row's mode is only ever set to StateModeIncr by Activate, and the initial seed is StateModeLegacy (0). The operator tool (in tools/botevent-seq/, outside review scope) presumably handles deactivation. N/A for this PR's three files.

C2 — Control-flow ordering / nested reuse:
The allocate → allocateFromCounter → afterIssue → gateClosed → allocate cycle is mutually recursive but bounded by maxAllocAttempts=3. I traced the attempt counter through every path: allocate(0) → allocateFromCounter(0) → gate-closed → gateClosed(1) → allocate(1) → ... → gateClosed(3) → error. Bounded correctly. Clear.

C3 — Authorization boundaries:
The allocator keys on robotID which the documentation states "must be the identity resolved from the authenticated context, never a request-body value." The allocator itself does not enforce this — it is a leaf package that trusts its callers. The Activate function is not exposed via any HTTP endpoint in these files; it is called by the operator tool. N/A for these files; caller responsibility.

C4 — Authorization lifecycle / state cascade:
The Activate function validates floor > observedMax before flipping, preventing activation with a floor below live cursors. The CAS guard (AND mode=0) prevents double-activation. The epoch is bumped on each activation, so a deactivate-then-reactivate produces a new epoch that won't match stale mirrors. Clear.

C5 — Build/note through is not sufficient:
Not applicable — these are pure Go files with no build artefacts, browser extensions, or packaging concerns. N/A.

C6 — Governance/policy/document self-consistency:
Not applicable — no governance documents, SECURITY.md changes, or policy files in these three files. N/A.

---

5. Concurrency Analysis

Shared state	Type	Protection	Verdict
activeBelief	atomic.Pointer[belief]	Immutable structs, copy-on-write	Correct
seeded	sync.Map	Concurrent-safe, cleared by invalidateSeeded	Correct
lastIssued	sync.Map	CAS loop in recordIssued (monotonic advance only)	Correct
lastPersisted	sync.Map	CAS loop in storeMonotonic (monotonic advance only)	Correct
seedLocks	sync.Map	LoadOrStore for per-bot mutex creation	Correct
beliefMu	sync.Mutex	Guards authority reads, held during DB call (300ms max)	Correct
authorityReads	atomic.Int64	Atomic increment	Correct
expectedMode	atomic.Pointer[expectedModeGuard]	Parsed once in init(), test hook swaps atomically	Correct
seedClientOnce	sync.Once	Singleton client creation	Correct
Lua scripts	Redis-side	Atomic by Redis single-threaded execution	Correct

No data races identified. The sync.Map.CompareAndSwap usage (Go 1.20+) is compatible with the module's go 1.25 declaration.

---

6. SQL Injection / Raw Query Safety

All SQL queries use parameterised binds:

• state.go:105: SELECT ... FROM state WHERE singleton_id=? — bind: stateSingletonID (constant 1)
• state.go:155: SELECT ... FOR UPDATE WHERE singleton_id=? — bind: constant
• state.go:173: UPDATE state SET mode=?, epoch=?, cutover_floor=? WHERE singleton_id=? AND mode=? — all binds
• seq.go:979: INSERT INTO seq ... ON DUPLICATE KEY UPDATE min_seq = GREATEST(min_seq, VALUES(min_seq)) — binds: HighWaterSeqKey(robotID), mark
• seq.go:1130: SELECT min_seq FROM seq WHERE key=? — bind: HighWaterSeqKey(robotID)
• seq.go:1148: SELECT min_seq FROM seq WHERE key=? — bind: constructed key

Table names (stateTable, "seq") and column names are all string constants. No string interpolation of user-controlled values into SQL. No SQL injection risk.

---

7. Cross-Round Blocker Re-Review (R6)

N/A — this is the first review pass on this PR.

---

8. Additional Observations

• I/O shape change is honestly documented. The comment at seq.go:398-403 explicitly states that every allocation is now a Redis round trip versus GenSeq's 999/1000 cache-hit rate. This is a real performance regression that the design requires (a block allocator would reintroduce #697). Load testing before activation is correctly recommended.
• The GenSeq chokepoint guard (genseq_guard_test.go) is a strong structural safeguard — it statically asserts that GenSeq for bot event IDs is called from exactly one site (legacyEventID). Any future developer adding a second call site will trip this test.
• The expectedMode env guard correctly fails closed on malformed values (OCTO_BOTEVENT_EXPECTED_MODE=inrc refuses rather than silently disabling). This is the same pattern as #627 and is well-tested.

---

Verdict

No P0 or P1 findings. All identified issues are P2 (improvements to error messages, memory hygiene, readability) or Nit. The code correctly implements its documented contracts:

• Monotonicity: enforced by Redis INCR + regression detection + seed floor.
• Uniqueness: enforced by Redis INCR atomicity + co-located counter and queue.
• Activation safety: enforced by FOR UPDATE + CAS in MySQL, atomic Lua gate, and the belief terminal-property invariant.
• Error handling: errors propagate on the critical path; only documented best-effort paths (high-water persist, state floor backstop) swallow errors, and both have alerting metrics.

[Octo-Q] verdict: APPROVE — no blocking defects found. The P2 findings above are optional improvements.

[yujiawei]

Code Review — PR #702 (octo-server)

Reviewed at head 2375708662d5e0b7650742e070e8df25f5ee727d, merge-base 40627cc0.
This is the 8th review round on this PR; the delta since the last reviewed head
(2b4aac06) is 4 commits — af86decb, 29c6787a, 12aee7c7, 23757086.

---

1. Spec compliance

Spec: ✅

Checked against .octospec/tasks/bot-event-score-monotonic/brief.md (Goal /
Load-bearing / Out of scope / Acceptance) and the round-7 findings this head claims
to close.

Missing (漏建): none. The two items absent from the diff are both agreed
narrowings, not gaps:

• the score-source guard is marked DEFERRED in the brief (line 166) and recorded as
  #704 Gap 4;
• rollback detection and the durable-mark bound are on #704, which rounds 5–7 all
  agreed to.

Extra (超建): one, non-blocking. .gitignore:96-102 adds ignore entries for four
other operator-tool binaries (msgextra-version, card-action-dlq, i18nmarkers,
genseq-repro) — strictly outside #697. This is the declined P2-1 nit; three lines,
defensive against a mistake this PR already made once, and a reason is given. Noted,
not held against the gate.

Deviation (偏离): one, P2. pkg/botevent/mode.go:178 states:

"Nothing in the allocator writes this key now; only the operator tool does"

The allocator does write it — pkg/botevent/seq.go:639, the mirror-rebuild branch:

if rebuildMirror {
    if err := client.Set(ModeKey, b.mirrorValue(), 0).Err(); err != nil {

Both the PR description and the brief's round-6 record keep the correct qualifier
("never writes the mirror on the legacy path"); only this comment dropped it.
Detail under Quality/P2-1. This is a comment-vs-code mismatch, not a code-vs-spec
one, so it does not fail this gate — but note it is the sixth consecutive round in
which a documentation claim overstated the code, and this round every other claim I
spot-checked was accurate (the added Redis round trip is now disclosed in Summary
beside the sentence it qualifies; invalidateSeeded's docstring now states the real
smaller stake; the EXPECTED_MODE refusal text no longer implies it is an override;
persistHighWater now says plainly that the mark trails without bound).

Verified claims:

• Queue-key consolidation is complete: grep -rn '"robotEvent:' modules/ pkg/ tools/
  outside _test.go returns exactly one hit, pkg/botevent/seq.go:244
  (QueueKeyPrefix). rb.robotEventPrefix and bot_api's constant are both gone.
• The three named tests exist and assert what the description says:
  TestNotActivatedDoesNotQueryTheAuthorityPerAllocation (seq_test.go:711),
  TestCounterKeyCannotCollideWithModeKey (:753),
  TestKnownResidualZaddReorderingCanStillSkip (:905).
• payload.robot_id really is client-only — no server-side path sets that message
  payload field, so the new existence check cannot break a server-generated flow.
• Round-7 P1-1 is genuinely fixed: dropMirrorSource / dropUnauthorizedMirror are
  deleted outright rather than guarded, and
  TestRegressedAuthorityDoesNotDestroyTheLegitimateMirror asserts the mirror
  survives a regressed authority.
• Round-7 P2-1 is fixed: refuseUnauthorizedMirror reads the authority before judging
  the mirror (tools/botevent-seq/main.go:382-392), and judgeMirror is split out as
  a pure function with the tool's first test file.

---

2. Code quality

Quality: Approved

No P0. No P1 reachable in the merged state. Findings below are P2, plus one that is
unreachable by merging and must be closed before activation.

P2-1 — the allocator still writes the mode mirror, and one comment says it doesn't

pkg/botevent/mode.go:178 vs pkg/botevent/seq.go:616-642

rebuildMirror := mirror != b.mirrorValue()
...
if rebuildMirror {
    if err := client.Set(ModeKey, b.mirrorValue(), 0).Err(); err != nil {   // :639

Two things:

1. The comment is wrong and it is load-bearing for a reader's model of who owns
   this key. Restore the "on the legacy path" qualifier the description and the brief
   both keep.

2. The write is unconditional, so a process holding epoch N will overwrite a
   mirror at epoch N+1 without ever re-reading the authority. If that happened, every
   replica validated against N+1 gets gateNotActivated, forces a refresh, and the
   mirror flaps — each flap calling invalidateSeeded() (seq.go:626), which drops
   every bot's seed marker and triggers a fleet-wide re-seed (3 DB reads + a
   ZREVRANGE + a Lua call per active bot).

   I do not think this is currently reachable, and that is why it is P2 rather than P1:
   the only writer of a newer epoch is botevent-seq, and Activate returns early
   once mode == StateModeIncr (state.go:164-166), so epoch never advances past 1
   without an unsupported manual UPDATE ... SET mode=0 — which the tool itself
   documents as having no online form (tools/botevent-seq/main.go:297). A
   compare-and-set that refuses to lower the epoch would make the invariant structural
   instead of depending on that argument holding.

P2-2 — the one read-storm the denial cooldown does not cover (blocker for activation, not for merge)

pkg/botevent/mode.go:500-511

The round-7 rationale for removing the mirror-delete is that "the delete's only benefit
was suppressing the read storm, and the cooldown suppresses that too" (mode.go:176-179).
The cooldown is armed in exactly one place — via noteMirrorUnauthorized at
seq.go:608 — which requires the authority read to have succeeded and said legacy.
The inconclusive branch does neither:

default:
    authorityUnreadable.inc()
    if prior != nil && prior.activated { ... }
    if mirrorClaimsIncr {
        return modeResolution{}, fmt.Errorf("botevent: the mode mirror claims activation but the "+
            "authority is unreadable (%w); refusing to allocate ...", err)   // no install(), no cooldown
    }

So with a mirror claiming incr and an unreachable authority, nothing is cached and
nothing is throttled. resolveMode (:397-406) will not short-circuit — the negative
TTL is skipped because the mirror claims activation, and deniedMirror is empty — so
every allocation calls refreshAuthority, which holds beliefMu across
readStateDeadlined (:427-428, :460). Each one costs up to authorityTimeout
(300ms), serialized, inside a held msgSem slot.

Concrete failure: an activated fleet, mirror incr:1 present and correct, one replica
restarts during a MySQL incident. That replica has no belief, so every bot-event
allocation blocks ~300ms behind one global mutex. 100 msgSem slots serialize to ~30s
and stay held — which stalls message fan-out for every bot in the process, not just
bot events. That is precisely the hazard aedde27a and the "Latency bound" section
exist to prevent, and it is the same class as P1-C and round-7 P1-1.

Why this is not a merge blocker. In the merged, pre-activation state there is no
botEventSeq:mode key, so mirrorClaimsIncr is false, the negative belief is installed
and cached for negativeBeliefTTL, and a MySQL outage fails enqueues exactly as it
already does (GenSeq is DB-backed too). Reaching this branch pre-activation needs a
forged mirror and an unreachable MySQL — a double fault in a state where enqueues are
failing regardless. It is reachable only after activation, which puts it in the same
category as the two exposures rounds 5–7 agreed to move to #704.

Ask: add this to #704 as a named gap before -action activate is run anywhere. The
cheap form is to install a belief (or arm a short refusal cooldown) in the inconclusive
branch so the refusal is fast rather than re-read per allocation; the refusal itself is
correct — only its cost is wrong.

P2-3 — permanent per-value state is bounded by shape, not by cardinality

modules/robot/event.go:57-74, modules/botfather/api_user.go:472-493

plausibleRobotID bounds length and rejects whitespace/control characters, but any
distinct 1–40-byte ASCII string still passes, and on the existRobot-error
fall-through (event.go:239-243) it is adopted. Each adopted value can become a
botEventSeq:counter:{id} key with no TTL under noeviction plus a seq row that is
never reclaimed.

I checked how reachable that actually is, and it is narrower than it looks:
existRobot only errors on a Redis error or a DB error, and in either case the
allocator's own I/O fails too — a Redis error kills probeAllocatorState; a DB error
kills legacyCeiling / highWaterCeiling (pre-activation, GenSeq itself). So the
enqueue is refused and no permanent state is created. Creating state needs a failure
selective enough that rb.db.exist fails while the allocator's queries succeed.
Negative results are also not cached, so there is no robot:exist:* pollution.

This is net hardening against main, which had no check at all, so it is not something
to hold the merge on. Worth pairing with #704's reclamation sweep — the sweep is what
actually bounds this, and the deleteUserBot comment already says so.

P2-4 — orphaned doc comment

tools/botevent-seq/main.go:302-308

writeMirror's docstring now sits immediately above type mirrorVerdict int, so godoc
attributes it to mirrorVerdict, and writeMirror (:403) has no comment at all.
Introduced by 29c6787a. Move the three lines down to :403.

P2-5 — the counter itself has no float64 ceiling

pkg/botevent/seq.go:1035-1038

botevent-seq refuses a cutover floor above 2^50 (maxSafeFloor), but runGate
accepts any positive id, and nothing stops the live counter from eventually issuing ids
where float64(seq) stops distinguishing adjacent int64s — which would recreate the
exact pagination skip and multi-member ack this change removes. Arithmetically
unreachable (one bot would need ~9×10^15 events), so a nit; noting it because the guard
exists on the floor and not on the value.

---

3. Overall verdict

APPROVE

Spec ✅ and Quality Approved. The merged state is inert — legacyEventID is still the
only allocator until an operator flips the authority row, the GenSeq guard fails if that
delegation is ever deleted as dead code, and the one behavioural delta on merge (a Redis
EVALSHA per pre-activation allocation, with addInlineQuery as the only genuinely new
dependency) is now disclosed in Summary rather than buried further down.

Merging is explicitly not activation clearance. #704 must close first, and P2-2
above should be added to it.

---

4. Recommendations

1. P2-1: restore "on the legacy path" in mode.go:178; optionally make
   seq.go:639 a compare-and-set that cannot lower the epoch.
2. P2-2: add the authority-unreadable + mirror-claims-incr read storm to #704 as
   a named gap. Smallest fix: install() the inconclusive belief (or arm a short
   refusal cooldown) before returning the refusal, so a restarted replica fails fast
   instead of paying 300ms under a global mutex per allocation.
3. P2-4: move writeMirror's docstring back onto writeMirror.
4. Stop iterating on this PR. Eight rounds is well past the point where further
   rounds cost more than they find: this round's delta is 4 commits, the one blocking
   finding from round 7 was fixed test-first, and everything above is P2. The remaining
   risk is not in the diff, it is in the activation procedure — so the productive move
   is to land this and give #704 a single owner with the activation checklist, rather
   than open a ninth round on comment wording. Flagging this for the maintainer as a
   process call, not a code one.

---

5. For a human to verify (this PR is labelled needs-human-review)

1. Production Redis configuration — highest priority. The whole co-recovery
   durability argument (seq.go:38-51), the "counter cannot be evicted" argument
   (:89-91) and the RDB-rollback safety margin all rest on appendonly no,
   maxmemory 0 / noeviction, and save 3600 1 300 100 60 10000, plus the counter
   living in the same instance and db as the queue. None of that is checkable from
   the repository, and if any of it is untrue the design's safety argument changes shape
   rather than degrading gracefully. Confirm before activation.
2. payload.robot_id is checked for existence, not authorization — pre-existing,
   correctly routed to an owner in an earlier round and not changed here. Any
   authenticated sender can still name any existing bot in a message payload and have
   an event enqueued onto that bot's queue. Independent of #697; still open.
3. The migration Down guard depends on MySQL ≥ 8.0.16 actually enforcing CHECK
   (modules/robot/sql/20260805000001_bot_event_seq_state.sql:48-52). The stated
   premise is that the project is pinned to MySQL 8.0; on an older server the Down
   would silently drop the authority row while mode=1. Verified by comment only.
4. modules/group and modules/botfather are reported as failing in the author's
   environment and also on the previous head. I did not reproduce either; CI is the
   authority.

---

6. Additional findings (outside the requested scope)

1. Two consumers of one queue with different cursor semantics.
   modules/bot_api/events.go:272 reads Min: fmt.Sprintf("(%d", eventID) — exclusive.
   modules/robot/api.go:1085 reads Min: fmt.Sprintf("%d", eventID) — inclusive.
   Pre-existing and untouched, but worth naming here because this PR substantially
   expands the eventPage docstring (events.go:174-219) to assert "the exclusive
   cursor" as a property of the queue. It is not a property of the queue; it is a
   property of one of its two readers. The two readers also merge different sources
   (getEventsResult in modules/robot folds in inlineQueryEventsMap), so the
   docstring's reasoning is correct for bot_api and does not transfer.
2. addInlineQuery writes to a process-local map while sharing a fleet-wide cursor.
   An inline-query event allocated on replica A lives only in A's
   inlineQueryEventsMap; if the bot then long-polls replica B and receives a queue
   event with a higher id, its cursor advances past the inline event, which A will then
   filter out forever. This is real, but it is not introduced or worsened here —
   before this PR addInlineQuery already called
   ctx.GenSeq(common.RobotEventSeqKey + robotID), i.e. the same sequence as the queue.
   The PR description documents an intermediate revision that gave inline queries their
   own key (which would have been much worse) and reverted it. Recording it as a
   pre-existing architectural issue worth its own issue, not as a finding against this
   diff.
3. Guard blind spot, cosmetic. chokepoint_guard_test.go:42 and :147 still match
   on robotEventPrefix, a field this PR deletes. Harmless — it keeps the guard able to
   recognise a reintroduced spelling — but it now matches nothing in the tree.

---

7. Review method and coverage limits

What I verified locally, in a clean worktree at the head SHA:
go build ./... clean; go vet clean on pkg/botevent, tools/botevent-seq,
modules/robot; gofmt -l clean on every file this PR touches; go test ./tools/botevent-seq/... ok; all source guards pass (chokepoint, GenSeq,
key-collision, and both "would this guard actually fail" tests).

What I could not verify. No MySQL or Redis in this environment, so TestMain
skipped ~40 integration tests — I confirmed the skip path fires with the documented
stderr note, which also confirms the CI=true hard-failure mechanism is wired the way
the description claims. Everything carrying the safety argument (seeding above all
three ceilings, activation with no cache window, rollback detection,
TestExclusiveCursorIsLosslessWithMonotonicIDs,
TestKnownResidualZaddReorderingCanStillSkip) is therefore unverified by me — I
read those test bodies and confirmed they assert what they claim, but CI is the
authority on whether they pass. Also unassessed: production Redis/MySQL configuration
(item 5.1), and the two packages the author reports as environmentally broken.

I ran two independent advisory passes alongside my own read. Both returned, and both
returned degraded, so I am naming that rather than treating their silence as
agreement:

• One pass reviewed the change via patch fetches rather than whole files and ran with
  reasoning disabled; it returned nothing at all on the belief-cache interleavings or
  the tool's activation ordering, which were two of the six areas it was asked about.
  It independently reached P2-1, which is why that finding is stated with the most
  confidence here.
• The other pass truncated at its token limit mid-finding and saw only the first 2000
  lines of a 5687-line diff, so most of seq.go, mode.go, state.go and the tool
  were never in its window. Two of its four findings were attribution errors against
  the pre-image — a guard regex it read as too narrow (the alternation's first branch is
  a bare substring that matches every spelling, and
  TestGenSeqGuardWouldCatchAReintroduction:130-131 pins exactly the shapes it worried
  about), and an if/else if control-flow claim that is identical in the pre-image — and
  I am not carrying either forward.

Neither pass covered state.go's Activate CAS, the migration's Down trick, or the
modules/group / modules/botfather producers; that gap is mine and I read all three
directly.

[yujiawei]

Follow-up to my approval above — non-blocking, no re-review needed, the approval stands. One additional P2 surfaced from a second pass over the regression-recovery path, plus a correction to a plausible-looking fix so it doesn't get applied as written.

P2-6 — afterIssue reports a sentinel as an id in its refusal message

pkg/botevent/seq.go:827-842

The post-regression retry handles one sentinel explicitly and lets the other fall through:

retried, err := runGate(client, robotID, b.mirrorValue())
if err != nil { return 0, err }
if retried == gateNotActivated {          // -1 handled
    return gateClosed(ctx, client, robotID, attempt+1)
}
if retried <= prev {                      // -2 lands here
    return 0, fmt.Errorf("...re-seeding only reached %d, which is still at or below the %d ...", retried, prev)
}

prev comes from checkRegression and is always a real issued id, so gateCounterMissing (-2) satisfies retried <= prev and the operator gets "re-seeding only reached -2". The refusal is correct — failing closed is the right outcome either way — but the message misattributes the cause: the counter key disappeared again mid-recovery, it is not a floor that failed to clear.

Worth fixing because the sibling call site already gets this right, which is what makes the asymmetry look accidental rather than intended — allocateFromCounter:671-673 uses if retried < 0 and names the sentinel:

return 0, fmt.Errorf("botevent: counter for %q still unusable after re-seed (gate=%d)", robotID, retried)

Suggest an explicit case gateCounterMissing in afterIssue before the <= prev comparison.

Correction on the per-bot map growth nit

seedLocks (and lastIssued) are indeed never cleared in production — invalidateSeeded:788 covers only lastPersisted and seeded, and the four-map sweep at :1158 is ResetSeededForTest. If that ever gets addressed, do not fix it by adding seedLocks to invalidateSeeded. reseed:897-909 holds seedMutex(robotID) across seedCounter and only stores seeded after it returns, so deleting the entry mid-flight lets a concurrent caller LoadOrStore a fresh mutex and run seedCounter in parallel — the double-check inside the lock cannot catch it because the winner has not stored seeded yet. That is exactly the racing-seeder case reseed's docstring exists to prevent ("burning a block of ids per racing caller"), and it would fire hardest during a mirror rebuild, when every active bot re-seeds at once. A fixed-size sharded lock pool keyed on a hash of the id has no such window.

For the record: at UUID-hex ids this is a few hundred bytes per bot ever allocated for in the process lifetime, so it is a hygiene note, not a leak worth a patch on its own.