Repository navigation
fix(kernel): restore checkpoint verify_max_retries on paused goal readout - #8359
Conversation
…dout
GoalRunner::state() reconstructs a paused run from its checkpoint once
the loop task has exited, and the checkpoint carries verify_max_retries
for exactly this reason: it is the one loop-engineering value the goal
document never holds, since it is a per-run number the operator sets on
the start body rather than part of the goal's own configuration.
Reading it from the goal document instead meant a run started with
{"verify_max_retries": 8} reported the compiled default once paused,
and the bodyless /resume that follows a readout resolves its own
budget the same way state() does, so a paused goal's operator-set
retry budget silently disappeared on resume with nothing failing.
This fix and its two regression tests shipped once already on the
branch that introduced pause/resume for loop-engineered goals, but
librefang#7973 was merged into main by squash while that branch was still
live, and the squashed commit that landed did not carry them.
The fragment was written before opening the pull request; GitHub assigned librefang#8359, not the librefang#8358 anticipated.
|
Correct fix for a defect that is on The defect is real.
The fix matches The test discriminates. A checkpoint at 8 read back as 8, with On the attributionThe changelog says the fix "shipped once already … but a squash merge on a long-lived branch left them out of the squashed commit that reached
Which is worth saying out loud rather than leaving implied: the loss is not visible in any conflict, so no resolution would have caught it. What catches this class is the test, and that is what the PR adds. Nothing blocking. |
houko
left a comment
There was a problem hiding this comment.
Reviewed, and I verified the load-bearing claim rather than taking it from the comment.
The fix says it resolves the budget "exactly as start() resolves it on the way back in". That is the whole argument — if it were off, the readout would just be wrong in a new way — so I read the other side:
// goal_runner.rs:1081, start()
verify_max_retries: if loop_engineering {
verify_max_retries
.or_else(|| resume.as_ref().and_then(|r| r.verify_max_retries))
.unwrap_or(DEFAULT_VERIFY_MAX_RETRIES)
.max(1)
} else { 0 }Explicit argument, then checkpoint, then compiled default, then .max(1). A bodyless /resume supplies no explicit argument, so it collapses to checkpoint → default → .max(1), which is what state() now does. The _ => 0 arm matches the else { 0 }. The claim holds.
Worth noting the one case the comment scopes correctly and I would have flagged otherwise: a /resume carrying an explicit verify_max_retries still overrides, so the readout and that resume can differ. That is right — the operator's new number should win — and the comment says "the bodyless /resume", not "resume", so it is not overclaiming.
The two tests are the right pair. The second is the one I would have asked for if it were missing: a checkpoint carrying 8 read against a goal whose loop_engineering was switched off while suspended must report 0, because reporting the number would advertise a gate the resume will not apply. Asserting max_iterations == 100 in the first test as the stated reference behaviour is a nice touch — it makes the fix "be consistent with the field two lines up" rather than a bare assertion.
One question, not a defect
The changelog says this shipped once already and a squash merge on a long-lived branch dropped it. I believe it, and it matches the shape of the bug.
If that is right, the interesting question is not this fix but whether anything would catch the next one. A regression that lands, then silently un-lands in a squash, leaves no trace: the tests went with it, so main is green and the behaviour is gone. Nothing here is wrong for not addressing that — it is out of this PR's scope by any reading — but it is the kind of thing worth a tracking issue if it has happened more than once, because the cost is paid a second time in full each time.
No blocking findings. This is a correct restore with the tests that should have been on main all along.
Summary
GoalRunner::state()(crates/librefang-kernel/src/goal_runner.rs) reconstructs a paused goal run from its persisted checkpoint once the loop task has exited and self-cleaned its registry slot. The checkpoint carriesverify_max_retriesfor exactly this reason: it is the one loop-engineering value the goal document never holds, because it is a per-run number the operator sets on the start body rather than part of the goal's own configuration.main,state()reads the retry budget asDEFAULT_VERIFY_MAX_RETRIES.max(1)instead of the checkpoint, so a run started with{"verify_max_retries": 8}reports the compiled default once paused. The bodylessPOST /api/goals/{id}/resumethat follows a readout resolves its own budget the same waystate()does, so the operator's number silently disappears on resume with nothing failing.start()resolves it on the way back in:checkpoint.verify_max_retries.unwrap_or(DEFAULT_VERIFY_MAX_RETRIES).max(1).a_paused_readout_reports_the_checkpoints_verify_max_retriesand its siblinga_paused_readout_reports_no_verifier_budget_without_loop_engineering, which pins that a goal without loop engineering still reports no verifier budget from the same checkpoint-reconstruction path.How this regressed
This fix and both regression tests already shipped once, on the branch (
feat/goal-pause-resume) that introduced pause/resume for loop-engineered goals.#7973merged that work intomainby squash while the branch was still live, and the squashed commit that landed did not carry this particular fix or its tests — a known failure mode of squashing a still-advancing branch, not anyone's mistake. Closes the gap: #7973Verification
Ported both tests first with
state()still inmain's form and confirmed the RED:(
a_paused_readout_reports_no_verifier_budget_without_loop_engineeringpassed even against the broken code, since that branch's_ => 0arm is unaffected — confirming it tests a distinct path.)Applied the fix, both tests went GREEN, then ran the full gate:
cargo nextest run -p librefang-kernel --no-fail-fast→ 2132 passed, 3 skippedcargo clippy -p librefang-kernel --all-targets -- -D warnings→ 0 warningscargo fmt --all --check→ clean