test(compression): pin the lease refresher's consecutive-failure give-up - #98867
JoaoMarcos44 wants to merge 1 commit into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact main@4f22543509d1b91dc45bcb369447126c5eb14fb7 → head@c999e2f28558a8a57580399cdbc42d5db4fc4716.
The real-SessionDB controls for owner revival, reclaimed-owner refusal, rollback, and live publication are useful. The final head's CI, Docker, and Nix workflows are green. This is not merge-ready yet.
Active ownership/interlock is missing
#98137 is open and already implements a competing symptom-B repair at the production publication boundary: its conversation_compression.py call passes require_lease_refresh=True, and its hermes_state.py change performs a holder-qualified in-transaction refresh before the expiry check.
This PR's publication controls call publish_compression_child with the default require_lease_refresh=False, so #98137 can make the production path commit while every control here remains green. The Scope section's open-PR census is therefore incomplete, and this suite does not constrain the active repair.
Interlock #98137 explicitly and choose one disposition: compose these controls into that repair's actual call shape, declare sequencing/ownership, or close/supersede this diagnostic PR. A single test that stops the refresher, expires the same lease, and invokes the production publication flags is the minimum useful composition.
The witness does not establish the reported run's provenance
FlakyDB establishes that repeated falsy outcomes can kill the loop. It does not establish that the reported Windows attempt actually experienced five consecutive refresh failures, nor that SQLite write contention caused them. The existing issue analysis deliberately left the concrete refresh-failure source unproven. Narrow the PR/commit/repro wording to a reachable mechanism unless refresh-attempt, latency, holder-mismatch, and expiry telemetry proves the observed attempt.
Also, #96768 is closed without merge; it is not a landed precedent for merging characterization tests that are expected to flip under a repair.
Repository proof gate
The final head is exact-green, but this branch contains two commits and 3963f1887bdc197646136444366fdbcd7069fdd7 has zero hosted check runs. Squash to one exact-green commit or provide exact-object proof for both commits before merge.
| permanent lost ownership, even while the compression it protects is | ||
| healthy and still running. | ||
| """ | ||
| window = _threshold(PROD_TTL_S, PROD_INTERVAL_S) * PROD_INTERVAL_S |
There was a problem hiding this comment.
This is the central arithmetic error. _run() performs the first refresh immediately and only then waits one interval. With a threshold of 5, failed calls occur at approximately t=0, 60, 120, 180, 240; the fifth failure breaks the loop. From the first failed refresh, the elapsed give-up time is therefore (threshold - 1) * interval = 240s, not threshold * interval = 300s.
The defect is real—and worse than described—but this assertion, the PR title/body, and the standalone repro pin the wrong schedule. Replace the local formula/constants with a deterministic driver of the real loop and assert the observed attempt times and break point.
There was a problem hiding this comment.
Fixed — thanks for catching this. Replaced the local formula assertion with TestGiveUpTiming, which drives the real _CompressionLockLeaseRefresher thread and measures the actual attempt timestamps: the 5th (break-triggering) failure lands at (threshold - 1) * interval, not threshold * interval. TestGiveUpWindow's restated formula is corrected to match (240s, not 300s). Pushed in 5aa81d17.
There was a problem hiding this comment.
Confirmed fixed on 5aa81d176e9d5737d53168fe24c5bfa1dde0e83b: the off-by-one formula is corrected and the real refresher loop’s immediate-first attempt schedule is exercised. The remaining production call-latency scope is separated in the current exact-head review: #98867 (review)
| row every delegated refresh returns False and the loop would break for | ||
| the wrong reason — making the counterfactual below pass vacuously. | ||
| """ | ||
| assert real_db.try_acquire_compression_lock( |
There was a problem hiding this comment.
This acquires the actual row with a 300-second TTL, but the refresher below is configured with TTL_S = 0.5. After the synthetic threshold failures, the database row is still live for almost five minutes, so this helper never reproduces refresher stops → lease expires → publication rejects.
The publication tests later expire a different 0.05-second lease, which splits the claimed causal chain across unrelated attempts. Use one TTL, holder, and row through acquisition, refresher failure, expiry, and publish_compression_child, preferably with a deterministic clock/event seam.
There was a problem hiding this comment.
Fixed. _drive() (and run_refresher() in scratch/repro_97948.py) now acquires the lock with the SAME TTL the refresher itself uses, instead of PROD_TTL_S, and accepts release=False so a caller can carry the exact row a give-up event left behind straight into publish_compression_child — one TTL, holder, and row through acquisition, refresher failure, and expiry, no synthetic 0.05s lease. This is now the driver for TestPublicationAfterRefresherGivesUp (the rebased publication class — see PR body). Pushed in 5aa81d17.
There was a problem hiding this comment.
Confirmed fixed on 5aa81d176e9d5737d53168fe24c5bfa1dde0e83b: acquisition, refresher failure, expiry, and publication now use one TTL, holder, and row, and the publication path exercises require_lease_refresh=True after #99216.
andrexibiza
left a comment
There was a problem hiding this comment.
The holder-revival, reclaimed-holder, and publication-rollback controls are useful. The central timing and causal claim is not established by this harness.
Blocker — threshold * interval == TTL is not the refresher’s wall-clock give-up window
_CompressionLockLeaseRefresher._run() performs the first refresh immediately:
first = True
while first or not self._stop.wait(self._refresh_interval_seconds):With five immediate falsy results, attempts occur at approximately t=0, 60, 120, 180, 240; the loop stops after about 240 seconds, not 300. The test test_give_up_window_equals_the_ttl only multiplies two configuration values; it never measures the production loop.
The write-contention claim diverges in the other direction. SessionDB.refresh_compression_lock() calls _execute_write() with the routine _WRITE_PATIENCE_S = 20.0 and converts an exhausted sqlite3.Error into False. Five real lock-contention failures therefore include the write patience for every attempt: roughly 5 * 20s + 4 * 60s = 340s, subject to scheduler/SQLite timing. A DB stall ending at 300 seconds can clear before the fifth attempt and recover. So “one TTL of write contention is fatal” is not what this code or test proves.
The durable hazard is real—five consecutive falsy outcomes permanently stop renewal—but the asserted 300-second causal window is not.
Blocker — this proves a possible mechanism, not the reported root cause
FlakyDB returns False directly. It does not drive SessionDB._execute_write, create real write contention, or record refresh timestamps/latencies. The issue’s total_duration_ms=710109 proves only that the compression ran long enough for multiple refresh opportunities; it does not prove that five refreshes failed. The existing issue analysis explicitly left the Windows refresh failure source unproven pending instrumentation.
Please frame this as a liveness hazard / candidate mechanism unless a real contention reproducer or production telemetry establishes the five failures.
Missing negative control for the repair contract
The proposed repair shape is to distinguish transient refresh failure from genuine ownership loss, but the refresher tests expose only one ambiguous False. test_a_reclaimed_lease_is_not_revivable calls SessionDB.refresh_compression_lock() directly; it never drives _CompressionLockLeaseRefresher after holder B reclaims the row. A repair that retries every False forever would flip the intended refresher tests and still pass that DB-only negative control.
Add two refresher-level paths:
- transient SQLite failure/contention clears while holder A still owns the row → renewal resumes;
- expired row is reclaimed by holder B → holder A’s refresher recognizes terminal ownership loss and stops without extending B’s lease.
That likely requires a typed refresh outcome (renewed / transient failure / ownership lost) or an equivalent holder-qualified check; a bare boolean cannot express the acceptance contract.
Verification
Exact head CI/Docker/Nix are green. The PR has two commits, however, and the first commit 3963f188… has no workflow runs, so there is not yet every-commit proof.
Once the timing claim, causal scope, and holder-loss negative control are corrected, these controls can usefully support symptom B without overclaiming closure of #97948.
|
Heads-up: #99216 (salvage of #98137's reviewer-endorsed subset) is armed to land the repair for the boundary your tests pin — the in-transaction pre-publication lease refresh, scoped Once it merges, your executable pins could have a second life as regression guards: the |
|
Topology changed since this diagnostic was opened. #98137 is now closed unmerged, and #99216 has salvaged the holder-qualified pre-publication refresh plus split-failure cooldown onto current That makes this PR diagnostic/provenance evidence, not an unclaimed production settlement. The existing exact-head review still applies: the Do not treat #98867 and #99216 as independent fixes. If this branch is retained, its useful role is a restacked diagnostic witness interlocked to #99216; the production settlement is owned there, while #97948 remains open for the separate terminal-settlement/late-ack symptom A contract. Live production-carrier review: #99216 (review) |
c999e2f to
5aa81d1
Compare
|
Restacked onto `main` (5aa81d17) — @kshitijk4poor / @andrexibiza:
I haven't touched the "no hosted workflow proof on first commit" point — that's CI history on the existing commits, not something a new commit can retroactively fix; happy to take direction on whether that needs anything further from me. |
Done ,My Commander |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact main@936b970e281d5d28e930c5698f36bc4ebb54c7ba → head@5aa81d176e9d5737d53168fe24c5bfa1dde0e83b.
The revised publication composition is now correct: _drive() carries one TTL, holder, and row through refresher give-up into publish_compression_child(..., require_lease_refresh=True), and the suite distinguishes retained-holder success from reclaimed-holder refusal. The #99216 ownership/scope statement is also corrected. CI, Docker, and Nix are green at this exact head. I am resolving the two superseded inline threads.
This is still not merge-ready.
The 240-second number is a zero-latency lower bound, not the production give-up window
TestGiveUpTiming timestamps entry into FlakyDB.refresh_compression_lock(), whose synthetic failures return immediately. The production method runs _execute_write() with _WRITE_PATIENCE_S = 20.0; _run() waits the 60-second interval only after each call returns. Under exhausted write contention, attempt starts are therefore approximately 0, 80, 160, 240, 320, with the fifth False returning around 340 seconds—not 240.
The current PR body, test docstrings, and standalone repro still say the refresher gives up one interval before the TTL and that write contention shorter than the TTL is fatal. This harness does not establish either claim. It establishes that five consecutive falsy outcomes stop renewal permanently, and that 240 seconds is the minimum inter-attempt wait when refresh calls have negligible latency. Reframe the title/body/docstrings and S4 accordingly, or add a controlled call-latency/contention witness and state the measured schedule. Keep the reported Windows event framed as a reachable candidate mechanism; FlakyDB still does not prove its actual refresh outcomes.
Commit history still fails the exact-object gate
The rebase preserved three commits. 69be6949debb6f85972e64b67106f61ae6bc1554 and 6ebe06259b9cee6f93823ef7aef4140471ea9285 have zero hosted check runs; both commit messages also retain the now-corrected exactly 300s and pre-#99216 publication-failure claims. Only 5aa81d176e9d5737d53168fe24c5bfa1dde0e83b is hosted exact-green.
The previously stated remedy remains: squash/reword this branch into one accurate commit, force-push, and wait for CI, Docker, and Nix to pass on that new exact head. An additive commit cannot retroactively verify or correct the two commits that would remain in merge history; rewriting them can.
Once both are corrected, the revised publication guard has no remaining code-level blocker from this review.
Symptom B of NousResearch#97948: a large-session compression runs for 11+ minutes and then aborts at publication with `Compression lease lost before publication` / `failure_class: session_split_failed`, rolls back, and the next turn re-triggers the same work. Tests and a standalone probe only; no production file is touched. NousResearch#99216 has already landed the production repair (an in-transaction, holder-qualified lease refresh immediately before publication), so this branch is diagnostic and provenance evidence interlocked to that fix plus a regression guard for it -- not an independent or competing settlement. What the controls establish --------------------------- `_CompressionLockLeaseRefresher._run` (agent/conversation_compression.py:2271) breaks permanently after `max(1, int(ttl / interval))` consecutive falsy refreshes -- 5 on production values (ttl 300s, interval 60s). The durable hazard is that COUNT: the loop never consults `expires_at` or elapsed time, and once it breaks there is no next tick. That contradicts `SessionDB.refresh_compression_lock` (hermes_state.py:7821), which decides ownership by the `holder` column alone precisely so a starved owner can revive its still-unclaimed row "on the next tick". A falsy return covers both a genuinely reclaimed row and a transient write failure, and the loop cannot tell them apart. The wall-clock window is a range, not a constant ------------------------------------------------ `_run()` starts waiting its interval only AFTER each refresh call returns, so the give-up window is `(threshold - 1) * (interval + call_latency) + call_latency`: * zero-latency floor -- instant failures land at 0, 60, 120, 180, 240, so the break-triggering 5th failure is at 240s, one interval short of the TTL; * contended ceiling -- `refresh_compression_lock` runs `_execute_write` on the routine `_WRITE_PATIENCE_S = 20.0` budget (hermes_state.py:4426) and converts an exhausted retry into `False`, so attempts start at 0, 80, 160, 240, 320 and the 5th `False` returns around 340s, PAST the 300s TTL. `TestGiveUpTiming` measures both ends on the real thread (a controlled call-latency witness, not a restated formula) rather than quoting either as the production window. Earlier revisions of this branch claimed `threshold * interval == 300s` and then a flat 240s; both are corrected here, along with the claim that write contention shorter than the TTL is fatal -- a stall clearing at 300s can still be caught by the 5th attempt. Provenance scope ---------------- `FlakyDB` returns falsy outcomes directly. It does not drive `_execute_write`, create real SQLite contention, or read production refresh telemetry, so nothing here proves the reported Windows attempt actually experienced five consecutive refresh failures. The 710s `total_duration_ms` shows only that the run outlived the whole window range. This is a reachable candidate mechanism, framed as such in the test docstrings and the probe. Contents -------- tests/agent/test_compression_lease_giveup.py -- 14 passed, ~13s: * TestGiveUpWindow -- threshold is 5; the zero-latency floor (240s) and the contended ceiling (340s) bound the window from either side * TestGiveUpTiming -- drives the real loop and measures both the instant- failure schedule and how the give-up point moves with call latency * TestRefresherStopsPermanently -- the loop exits at the threshold, grants nothing afterwards, and recovers fully with one fewer consecutive failure * TestOwnershipStaysRecoverable -- an expired lease is still revivable by its owner, and a genuinely reclaimed one is correctly refused * TestPublicationAfterRefresherGivesUp -- regression guard for NousResearch#99216: drives the real refresher to its real give-up point over one TTL/holder/row, then asserts `publish_compression_child(..., require_lease_refresh=True)` now succeeds when nobody stole the lock, and still refuses when a different holder genuinely won the row scratch/repro_97948.py -- the same walk as a standalone 4-stage probe. Two harness defects found while writing this, both fixed here: `__init__` floors `refresh_interval_seconds` at 0.1s before deriving the threshold, so a test passing a smaller interval measures a threshold it did not declare; and the driver originally acquired the lock with a different TTL than the refresher used, so the row never reached the expired state publication needed to check. Refs NousResearch#97948 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EKrRS7LVgyHf2WQkEahSwu
5aa81d1 to
dbc5e05
Compare
|
Both blockers from the exact-head review addressed on 1. The 240s number is now framed as the floor it is, and the range is measured. You're right that Both ends are now stated and asserted against the real constants rather than magic numbers:
I took the "or add a controlled call-latency witness and state the measured schedule" option as well, since the arithmetic alone was what got this wrong twice. Two claims are withdrawn rather than restated: "the refresher gives up one interval before the TTL", and "write contention shorter than the TTL is fatal" — a stall clearing at 300s can still be caught by the 5th attempt, per your The provenance caveat is now explicit in the module docstring, the 710s test's own docstring, and the probe header: 2. Exact-object gate: squashed to one commit.
Local: 14 passed (~13s), |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed the exact replacement object dbc5e053a92ce131ad717d963dbf2babc85c60ae against current main@38b93e0abec1eb4198ea3d18a9cb607ed9745906. The PR remains mergeable; the head is now one commit and changes only the two declared test/probe files.
The two remaining blockers from my prior exact-head review are closed.
-
Timing and provenance: 240 seconds is now correctly framed as the zero-call-latency floor rather than the production give-up window. The branch carries refresh-call latency through the schedule, exercises both zero and controlled nonzero latency on the real refresher thread, and keeps the Windows report at candidate-mechanism provenance. The publication guard still carries one real row, holder, and TTL through refresher give-up into
require_lease_refresh=True, covering both the unclaimed-row success path and the genuinely reclaimed-owner refusal path. -
Exact-object history and CI: the stale three-commit stack is gone.
dbc5e053a92ce131ad717d963dbf2babc85c60aeis one accurately worded commit. The required hosted workflows completed successfully at this exact head:
The two earlier CI records attached to this SHA never instantiated jobs; they are not failed test executions. The completed CI rerun includes attribution, ruff, Python tests, CLI/setup e2e, Docker, Windows, macOS, docs, and the final required-check gate.
No remaining blocking finding from this review. Production settlement remains #99216; this PR is correctly scoped as the diagnostic/provenance evidence and regression guard, while #97948 remains open for symptom A.
Summary
Executable controls for symptom B of #97948 — the
Compression lease lost before publication/failure_class: session_split_failedabort that rolls a large-session rotation back and leaves the next turn to re-trigger the same work.Tests only; no production file is touched. #99216 has already landed the production repair, so this branch is diagnostic/provenance evidence interlocked to that fix, plus a regression guard for it — not an independent or competing settlement. See Scope.
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[🔒 Compression Acquires Lease<br/>ttl = 300s] --> B[⚡ Lease Refresher Thread<br/>interval = 60s, first tick at t=0] B --> C{🧬 Refresh Returned True?} C -->|yes| D[♻️ Reset Failure Counter] D --> B C -->|no| E[⌛ consecutive_failures += 1] E --> F{🔻 failures >= ttl / interval = 5} F -->|no| B F -->|yes| G[⛓️ break — Loop Dies Forever<br/>THE RULE IS A COUNT, NOT A CLOCK] G --> T1[📉 Zero-Latency Floor<br/>attempts at 0,60,120,180,240<br/>give-up at 240s — one interval under TTL] G --> T2[📈 Contended Ceiling<br/>each False burns _WRITE_PATIENCE_S = 20s<br/>attempts at 0,80,160,240,320<br/>5th False returns ~340s — PAST the TTL] T1 --> H[🕳️ Lease Left Untended] T2 --> H H --> I[🛰️ publish_compression_child<br/>in-transaction pre-publication refresh<br/>#99216, merged] I --> N{🔑 Row Still Holder's?} N -->|yes: nobody stole it| O[✅ Refresh Succeeds In-Transaction<br/>Rotation Publishes — #99216's fix] N -->|no: genuinely reclaimed| J[⛔ CompressionSessionBusyError<br/>Compression lease lost before publication] J --> K[🌑 Rotation Rolled Back<br/>session_split_failed] K --> L[🔁 Next Turn Repeats<br/>the same doomed compression] M[📡 refresh_compression_lock contract<br/>ownership by holder column ALONE<br/>a starved owner MUST revive<br/>on the next tick] -.->|row is still revivable<br/>the loop stopped asking| GMechanism
_CompressionLockLeaseRefresher(agent/conversation_compression.py:2271) keeps the durable lock alive while the summary streams, so a healthy 11-minute compression should never reach publication without a lease._run()stops permanently after_max_consecutive_failures = max(1, int(ttl / interval))consecutive falsy refreshes:Production values —
_lock_ttl = 300.0(conversation_compression.py:3332) and the derivedinterval = max(1.0, min(60.0, ttl / 2.0)) = 60.0— give a threshold of 5.The durable hazard is that count, not any particular wall clock. The loop never consults
expires_ator elapsed time; five consecutive falsy outcomes stop renewal permanently, whenever they happen.The wall-clock window is a range
_run()starts waiting its interval only after each refresh call returns, so the give-up window is(threshold - 1) * (interval + call_latency) + call_latency:0, 60, 120, 180, 240refresh_compression_lockruns_execute_write(_do)on the routine_WRITE_PATIENCE_S = 20.0budget (hermes_state.py:4426) and converts an exhausted retry intoFalse0, 80, 160, 240, 320Two claims are therefore withdrawn from earlier revisions of this PR, per review:
"the refresher gives up one interval before the TTL"— 240s is the minimum, reached only when refresh calls cost nothing."write contention shorter than the TTL is fatal"— a stall that clears at 300s can still be caught by the 5th attempt and recover.TestGiveUpTimingmeasures both ends on the real thread — a controlled call-latency witness, not a restated formula.Why this is a defect, not a policy
SessionDB.refresh_compression_lock(hermes_state.py:7821) decides ownership by theholdercolumn alone, never byexpires_at, and says why in its own docstring:The row stays revivable. The loop guarantees there is no next tick.
refresh_compression_lockreturnsFalsefor a genuinely reclaimed row and for a transient write failure, and the loop cannot tell them apart — that ambiguity is the whole defect, and it holds at either end of the timing range.Provenance scope
FlakyDBreturns falsy outcomes directly. It does not driveSessionDB._execute_write, create real SQLite contention, or read production refresh telemetry, so nothing here proves the reported Windows attempt actually experienced five consecutive refresh failures. Itstotal_duration_ms: 710109shows only that the run outlived the whole window range.This is a reachable candidate mechanism for that report, not its established root cause, and it is framed that way in the module docstring, the affected test docstrings, and the probe's S4 output.
What the controls pin
tests/agent/test_compression_lease_giveup.py— 14 passed, ~13s.TestGiveUpWindowTestGiveUpTimingTestRefresherStopsPermanentlyTestOwnershipStaysRecoverableTestPublicationAfterRefresherGivesUppublish_compression_child(..., require_lease_refresh=True)now succeeds when nobody stole the lock (regression guard for #99216), and still refuses when a different holder genuinely won the rowTestGiveUpWindow/TestGiveUpTiming/TestRefresherStopsPermanently/TestOwnershipStaysRecoverablepin the refresher-thread defect itself — #99216 never touches_CompressionLockLeaseRefresher._run.TestPublicationAfterRefresherGivesUpis the piece that guards the fix.scratch/repro_97948.pyis the same walk as a standalone 4-stage probe (python scratch/repro_97948.py) for anyone who wants it without pytest; S4 now prints both bounds and the measured latency-dependent window.Two harness defects found while writing this
Both are in the test harness, not in Hermes, and both are fixed here — recording them because either one silently weakens the evidence:
__init__floorsrefresh_interval_secondsat0.1sbefore deriving_max_consecutive_failures, so a test that passes a smaller interval measures a different threshold than the one it declares._drive()andrun_refresher()now acquire with the refresher's own TTL, and acceptrelease=Falseso a caller can carry the exact row a give-up event left behind through topublish_compression_child.Scope
Refs #97948, notCloses. This covers symptom B's lease loss only./compressreports a 120s timeout while the background worker succeeds minutes later) is a separate defect; @andrexibiza already traced it to the hard-coded budgets inapps/desktop/.../slash.tsandtui_gateway/methods_session.py. Manual /compress reports 120s timeout while background worker succeeds minutes later; large-session compression fails with lease lost / session_split_failed #97948 stays open for it.publish_compression_childtakes one final in-transaction lease refresh (WHERE session_id AND holder, same conn as the expiry check) immediately before publication. This PR is diagnostic/provenance evidence plus a regression guard for that fix — seeTestPublicationAfterRefresherGivesUp, complementary to fix(compression): in-transaction lease refresh before publish; cooldown on split failure (#97948 symptom B) #99216's own wrong-holder unit tests intests/state/test_compression_lease_refresh_before_publish.py, since it exercises the real background-thread timing those row-seeded tests don't.Test plan
tests/agent/test_compression_lease_giveup.py— 14 passedpython scratch/repro_97948.py— all four stages PASSruff check— cleangit diff --statagainst the merge-base is two test/scratch files)