batch_operator: report the group actually on duty when logging a non-election - #580
batch_operator: report the group actually on duty when logging a non-election#580heifner wants to merge 2 commits into
Conversation
…election The not-elected log printed `epoch_index % 3` as `active_group` — the static-rotation value from the design the sliding window replaced, and the exact shape `.claude/rules/batch-operator-schedule-window.md` names as the anti-pattern to look for. The decision it narrates compares `my_group` against `epochstate.current_batch_op_group`, which the window keeps pinned at 0 because `advance` pops the expiring group off the front, so the printed value has nothing to do with the branch being reported. The two agree only when `epoch_index % 3 == 0`. Every other epoch the line reports a group that was never consulted, and it reads as a contradiction whenever the printed value happens to equal `my_group` -- "not elected (my_group=1, active_group=1)" appears in the flow logs, and is what sent one investigation looking for a bug in the election itself rather than in the message describing it. `cur_group` is a local in `parse_epoch_state`, while the log sits in `do_poll_epoch_state`, which is why the original reached for an expression instead of the value. Retain it as `current_group` next to `my_group` -- both are per-poll election state, and `current_group_members` already establishes the name -- and print that. No behavior change: `is_elected` is computed exactly as before. Change-Id: I308c5194bf4bea51641cd42a48ba62fa47fe81e5
huangminghuang
left a comment
There was a problem hiding this comment.
One non-blocking consistency comment; the functional diagnostic fix looks sound.
huangminghuang
left a comment
There was a problem hiding this comment.
Approved. The existing P3 sentinel comment is non-blocking; the functional diagnostic fix is sound.
`GROUP_NONE` already named the 255 group sentinel and `parse_epoch_state` already assigned it to `my_group`, but both member declarations carried the bare literal — so `current_group` arrived as a second, unnamed copy of the same value, free to drift from the one the assignment path uses. Both declarations now initialize from `GROUP_NONE`, and its comment covers both meanings it carries: "we are not in any batch-op group" for `my_group`, "no epoch state parsed yet" for `current_group`. No behavior change — same value, named. Change-Id: I0bc5bdf1c6923957f6ce56c93c0a5f77b98b1768
| @@ -492,7 +498,7 @@ struct batch_operator_plugin::impl { | |||
| if (!is_elected) { | |||
| if (epoch_index != current_epoch) { | |||
| ilog("batch_operator: not elected for epoch {} (my_group={}, active_group={})", | |||
There was a problem hiding this comment.
[P3] Add regression coverage for the corrected active-group diagnostic This PR fixes a reproduced mismatch, but no test exercises this logging path; the listed validation is syntax-only compilation, and the existing plugin tests do not reach parse_epoch_state / do_poll_epoch_state. Please add a regression case with epoch_index = 1, current_batch_op_group = 0, and this operator in group 1, then assert the message reports active_group=0. Otherwise the old modulo expression could return unnoticed, contrary to the repository’s complete-test-coverage rule.
Follow-up to #573 (closed). That PR's backfill regressed
flow-batch-operator-terminationand carried two unresolved[P1]s, so it was closed rather than iterated. Two items surfaced during that investigation were real and independent of the backfill; this PR carries the one that is unambiguous.The fix
The not-elected log printed
epoch_index % 3asactive_group:That is the static-rotation value from the design the sliding window replaced — the exact shape
.claude/rules/batch-operator-schedule-window.mdcalls out:The branch it narrates compares
my_groupagainstepochstate.current_batch_op_group, which the window keeps pinned at 0 (advancepops the expiring group off the front, so the front is always the group on duty). The printed value is not the one the decision used.The two agree only when
epoch_index % 3 == 0. Otherwise the line names a group that was never consulted — and it reads as a flat contradiction when the printed value happens to equalmy_group. From a real flow run:Equal values, yet "not elected". Nothing is wrong with the election —
my_group == 1,cur_group == 0, correctly not elected — only with the message. That apparent contradiction is what sent one investigation looking for a bug in the election path rather than in the line describing it.cur_groupis a local inparse_epoch_state, while the log sits indo_poll_epoch_state, which is presumably why the original reached for an expression instead of the value. This retains it ascurrent_groupbesidemy_group— both are per-poll election state, and the existingcurrent_group_membersestablishes the name — and prints that.No behavior change.
is_electedis computed exactly as before; only the diagnostic changes.Not included, deliberately
The other item was the pre-mutation pool read in
sysio.epoch::advance—slashop/recorddel/termcheckare inline actions that execute only afteradvancereturns, so the pool scan reads operators queued for demotion as still ACTIVE.It is not carried here, because master already documents that ordering as a deliberate invariant:
And since #569,
sysio.msgch::eligible_batch_operators()intersects the seated group with live opreg ACTIVE status and sizesgroup_sizefrom the result, so a seated-then-slashed member is excluded and the threshold adapts rather than becoming unreachable.Changing that ordering means touching an invariant whose own comment warns the alternative aborts
advanceand stalls OPP advancement chain-wide. That is a design discussion, not a drive-by change — raising it separately.Validation
Syntax-only compile of the patched translation unit against the existing release build's flags: clean. No contract, ABI, or WASM changes, so the post-contract-refactor rebuild chain does not apply.