Skip to content

fix(gas_manager): set_limits must pause when tightened limit is below current spend#120

Merged
abhicris merged 2 commits into
kcolbchain:mainfrom
kite-builds:fix/gasmanager-set-limits-pause-consistency
Jun 9, 2026
Merged

fix(gas_manager): set_limits must pause when tightened limit is below current spend#120
abhicris merged 2 commits into
kcolbchain:mainfrom
kite-builds:fix/gasmanager-set-limits-pause-consistency

Conversation

@kite-builds

Copy link
Copy Markdown
Contributor

Summary

GasManager is documented as a drop-in replacement for both GasBudgetTracker and GasTracker "without behavioural surprises" (issue #97). It isn't, for one path: tightening a wallet's limit below its current spend via set_limits() leaves is_paused() and can_spend() disagreeing.

_update_pause_state_locked early-returned unless the wallet was already paused:

def _update_pause_state_locked(self, ledger, limits):
    if not ledger.paused:
        return        # <-- can only CLEAR a pause, never SET one
    ...

So lowering a limit below the recorded spend never flips paused to True. The reference GasTracker._update_pause_state recomputes the flag in both directions (_is_paused = not (can_proceed_hourly and can_proceed_daily)).

Impact

A consumer migrating from GasTracker that gates dispatch on the compatibility alias is_paused() after tightening a wallet budget will believe the wallet is healthy and keep sending, while can_spend() (correctly) rejects. Budget-enforcement state becomes internally inconsistent.

Reproduction (before fix)

m = GasManager(default_limits=GasLimits(per_hour=200))
m.record("0xA", 150)                       # healthy
m.set_limits("0xA", GasLimits(per_hour=100))   # tighten below the 150 spent
m.is_paused("0xA")     # -> False   (wrong)
m.can_spend("0xA", 1)  # -> False   (correct)  => disagree

The reference GasTracker reports is_paused() == True after the equivalent operation.

Fix

Recompute the pause flag in both directions after evicting aged events (set_limits previously also skipped eviction, so the sums it read could be stale). Loosening a limit still unpauses; tightening below current spend now pauses.

Tests

  • test_set_limits_below_spend_pauses_and_stays_consistent (per-hour)
  • test_set_limits_per_day_below_spend_pauses (per-day)

Both fail on main (is_paused() returns False), pass with the fix. Existing test_pause_on_hit_release_on_set_limits / test_pause_stays_if_new_limits_still_exceeded still pass. Full Python suite: 220 passed, 62 skipped (1 pre-existing unrelated flask import failure in test_x402_server), gas_manager.py ruff-clean.

🤖 Generated with Claude Code

…ow current spend

GasManager._update_pause_state_locked early-returned unless the wallet was
already paused, so it could only ever *clear* a pause, never set one. Lowering
a wallet's limit below its recorded spend via set_limits therefore left
is_paused() reporting False while can_spend() (correctly) returned False — the
two disagree.

This breaks the documented drop-in parity with GasTracker, whose
_update_pause_state recomputes the flag in both directions
(_is_paused = not (can_proceed_hourly and can_proceed_daily)). A consumer that
gates dispatch on the GasTracker-compatible is_paused() after tightening a
budget would keep sending while the budget is actually exhausted.

Fix: recompute the pause flag in both directions after evicting aged events
(set_limits previously also skipped eviction, so the sums it read could be
stale). +2 regression tests covering the per-hour and per-day tighten paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@abhicris

abhicris commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Audit verdict: safe

Straightforward bug fix correcting an inconsistency where pause state and spend checks disagreed; adds appropriate test coverage with no malicious patterns, credential leaks, or supply-chain risks.

Audited by the kcolbchain PR pipeline. See pipeline docs.

The pause-direction regression tests assert that tightening a limit below
current spend pauses the wallet, but the _update_pause_state_locked docstring
contracts BOTH directions. Add the mirror case: a wallet paused by a too-low
limit must unpause (and is_paused() must agree with can_spend()) when the limit
is raised back above spend. Guards against a future regression restoring only
the pause half.
@kite-builds

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up: added the mirror unpause regression test (test_set_limits_above_spend_unpauses_consistently) so both directions of the _update_pause_state_locked contract are covered, not just the pause direction. Local suite 35/35 green.

Heads-up on the earlier red CI: it wasn't this change — the forge test job failed during foundryup install with a 504 Gateway Timeout fetching the foundry attestation artifact (forge test never actually ran). This is a Python-only diff; the new push should re-trigger a clean run.

@abhicris abhicris merged commit 3bf845a into kcolbchain:main Jun 9, 2026
2 checks passed
@abhicris

abhicris commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Merged — thanks, @kite-builds. That's 7 merged PRs to kcolbchain now.

You're a Fellow — when paid research, partner-org work, or grant milestones come up, you're in the first invite pool.

Next-up issues across the org: https://kcolbchain.com/invitations/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants