Skip to content

test(manager): detect overlapping lease renewals - #1327

Merged
davidfarah2003 merged 2 commits into
mainfrom
fix/1320-lease-renew-gradable
Sep 6, 2026
Merged

test(manager): detect overlapping lease renewals#1327
davidfarah2003 merged 2 commits into
mainfrom
fix/1320-lease-renew-gradable

Conversation

@davidfarah2003

Copy link
Copy Markdown
Contributor

Summary

The lease-renew D3 cell was ungradable on main, not only on a pull request. It returned the same UNGRADABLE verdict in main run 33981431625, in #1301, and in #1302.

The first commit adds a positive control against the same manager.ts guard line. The control was killed while D3 remained green, converting D3 to a reportable SURVIVED verdict and proving the gap was real rather than a reachability artifact.

The second commit closes that coverage gap. The old assertion looked for the absence of a wrong last sequence diagnostic. That could not distinguish a genuine lack of conflict from a conflict whose evidence never arrived. During the measured window, the stall also holds the broker's rejection, and the manager suppresses repeated same-state diagnostics. The smoke now counts lease update publish frames at the relay, observing the overlapping renew attempt itself instead of a downstream symptom that may never be emitted. The existing NO SELF-INFLICTED CAS CONFLICT cell name and D3 expectRed remain unchanged.

This unblocks #1301 and #1302. Both are approved at their exact heads and were held solely by this fixture.

Validation

The unmutated suite passes all 17 checks. The final committed whole-fixture reproof reports:

  • Positive control: KILLED, 7 marks against baseline 17, at [acklost] the child manager started, acquired its own per-instance lease, and renewed it at least once
  • D3: KILLED, 16 marks against baseline 17, at NO SELF-INFLICTED CAS CONFLICT
  • D2a: KILLED, 16 marks against baseline 17, at THE RENEW BUDGET HAS SLACK
  • D2b: KILLED, 16 marks against baseline 17, at THE RENEW BUDGET HAS SLACK

Also run:

  • pnpm build
  • pnpm smoke:lease-renew
  • pnpm smoke:mutation-fixtures
  • pnpm changeset status

A mutation-proof process terminated by an outer harness timeout leaves the current mutation applied because restoration only runs when the mutation runner finishes. Two timed-out validation attempts required explicit source restoration and a core rebuild before continuing.

Closes #1320

@davidfarah2003

Copy link
Copy Markdown
Contributor Author

APPROVE — head d1e86b2c99fc12b6c8e1889759a992aaf437c9dd

Independent review from outside the author's model family. I probed the load-bearing claim rather than only reading it; the negative control is the spine of this verdict.

§1 — CENTRAL PROBE (RAN, twice). The PR's account of itself is correct.
I reverted only lease-renew-no-answer.smoke.ts to its 820261798 state, kept the four-cell fixture, committed, and ran the fixture. Result matched the prediction exactly:

  • POSITIVE CONTROL → KILLED (8 marks, named [acklost] ...renewed it at least once). Reachability comes from the fixture cell, not the suite edit.
  • D3 → SURVIVED (17 marks, baseline 17) — with the old !/wrong last sequence/ assertion the overlap goes undetected. The gap is real and the suite edit is what closes it.

Positive complement (also RAN), new suite at the PR head:

  • D3 → KILLED (16 marks, named NO SELF-INFLICTED CAS CONFLICT).

So the suite edit is precisely what converts D3 from a genuine SURVIVED into a KILLED, and reachability is supplied by the new positive-control cell. World-2-then-closed is verified, not asserted. (First §1 run was killed by an outer 600s harness cap mid-D2b, leaving streams.ts mutated; I restored it by hand — see the body's own outer-cap warning, now confirmed firsthand.)

§0 — relabel check RE-VERIFIED. All four expectRed strings are byte-identical between 820261798 and head; D3 still names NO SELF-INFLICTED CAS CONFLICT, D2a/D2b still name THE RENEW BUDGET HAS SLACK. Nothing was relabeled to whatever reddened. D3 anchor if (this.leaseRenewInFlight) return;\n resolves exactly once on head AND on current origin/main (moved to line 2020, guard byte-identical); the control find matches only that line, not the field init at 974. streams.ts untouched by the PR. Diff is 3 files / 27 insertions / 5 deletions.

§2 — new assertion can fail and fails for the right reason. renewAttemptsDuringStall.length === 1 reddens on an empty set (right shape, unlike the old absence test). leaseSubject = $KV.${managerBucket(space)}.${managerLeaseKey(instanceId)} is the correct JetStream KV write subject; the same two helpers are used to READ the lease successfully in the suite, and baseline is green at 17 (a wrong subject would empty the set and fail the unmutated suite). Counting the KV update publish at the relay instead of parsing an error string is justified: the manager renews via renewManagerLease (one KV update per tick), the guard bounds it to one in-flight, so exactly 1 publish during the window and 2 without the guard. The "3/3 before the guard" note is a small sample, but the assertion is a structural count, not a race on wall-clock ordering, so I do not read it as timing-fragile.

§3 — widened relay is a pure pass-through when no drop filter is set. With dropMatch undefined, !dropMatch short-circuits so every complete frame is written and every non-PUB line / unparseable-length line passes through, in order; a partial frame is buffered until complete (return). Concatenation of the per-frame writes equals the original byte stream in order — the only change is chunk boundaries, which are already insignificant over TCP. publishes is per-connection and bounded by the run's own frames (no unbounded growth across cells). Every non-drop cell now parses instead of raw-forwarding, but no cell's meaning changes and the added assertion is the only new grade. Confirmed empirically: with the new relay, baseline stayed green at 17 and every mutation graded as predicted.

§4 — control's expectRed is the earliest red. The first check() in the suite is the child-start cell [...] renewed it at least once; the inverted guard makes the manager never renew, so that first behavioural assertion is the earliest to redden. Observed at 8 marks in my run (author reported 7); nothing structurally precedes it.

§5 — housekeeping. Body closes exactly one issue (Closes #1320); no stray closing keyword. Changeset .changeset/lease-renew-manager-control.md is empty frontmatter + one description line — the house pattern for test-only machinery, correctly bumping no package. Outer-cap restore warning is present in the body and I hit it myself. I did not grade the 205-solo-cell census (out of scope); the body does not imply otherwise.

CI (enumerated by name at the full 40-char sha). Mutation reproof → success (the whole-fixture reproof of this fixture, in CI). Windows → success. Code Quality → success. CI still in_progress (smoke shards 0-3/4 running; unit/live/changed/CodeQL all success). No workflow is failed. Docs is legitimately absent: the changed paths (.changeset/**, implementations/**) match none of docs.yml's paths: filter. mergeStateStatus = UNSTABLE, consistent with required checks still running, not with a failure.

What I could not check: the four-cell whole-fixture run to a single committed green in my worktree (each cell is a core build + suite; the outer harness caps me below the ~510s+ it needs here). I substituted the two-probe design above plus CI's own Mutation reproof success, which together cover the same claim more sharply. Worktree left clean at the head sha; both mutated sources (manager.ts guard, streams.ts MANAGER_LEASE_ATTEMPT_MS) confirmed restored; no processes left running.

@davidfarah2003
davidfarah2003 merged commit 13b5484 into main Sep 6, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mutation-proof: a file with exactly one cell in a fixture cannot be graded when that cell goes quiet

1 participant