Skip to content

perf(llc): the disposal sweep rereads its whole approval history every hour, and acts on decisions that have aged out #17738

Closed

Description

@mrveiss

Problem

The workspace disposal sweep (#16818, PR #17725) reads its approved proposals like this:

select(Approval).where(
    Approval.approval_type == ApprovalType.DESTRUCTIVE_ACTION.value,
    Approval.status == ApprovalStatus.APPROVED.value,
    Approval.requested_by_agent == SWEEP_REQUESTER,
).order_by(Approval.decided_at)

Nothing ever leaves that set. An executed proposal keeps status = APPROVED — execution is
recorded in context["executed_at"], because the approval vocabulary is six values shared by every
gate and inventing a seventh to suit one sweep would change all of them. So every proposal this
sweep has ever raised and had approved stays matched, for ever, each carrying its workspace list and
its execution_outcomes.

The sweep runs hourly and deserialises that entire history every tick to find the few unexecuted
rows. At roughly 24 rows a day that is tens of megabytes per tick after a year, growing monotonically.

It will not fail. It is the same shape as the defect PR #17725 just fixed — a query whose cost is
governed by history rather than by work — on a gentler slope.

Why a time window is not the bound that was just removed

The LIMIT removed in #17725 bounded which rows the filter could see, so executed rows could
crowd out the unexecuted one and silently stop all disposal. A window bounds which rows are
eligible at all
: an approval outside it is not hidden from the filter, it is genuinely not a
candidate. Those are different operations and only the first can starve.

The reason this is a correctness fix, not only a cost one

PR #17725 re-proves landedness at execution rather than trusting the proposal, on the argument that
an approval authorises the removal; it does not vouch for the evidence. That handles the
evidence going stale. It does nothing about the decision going stale.

Someone who approved a workspace removal six weeks ago approved it on the state they were shown
then. Re-proving landedness today does not make that a current authorisation — it makes it a current
fact attached to an old decision. A window makes the PR's own claim literal: an approval has a
lifetime, and past it the sweep asks again rather than acting on consent that has aged out.

Acceptance criteria

  • _approved_proposals filters on Approval.decided_at >= now - EXECUTION_WINDOW_DAYS.
  • EXECUTION_WINDOW_DAYS is an env-var-backed module constant, measured in days — a window
    in hours could drop an approval at the edge during a sweep outage, which is a silently unactioned
    human decision.
  • A decision whether an aged-out approval is skipped or marked ApprovalStatus.EXPIRED.
    Marking is preferable if the queue surfaces it, because a skipped approval is invisible to the
    person who granted it; skipping is acceptable if nothing reads that state. Record which and why.
  • A test asserts an approval older than the window is not executed, and one inside it is.
  • The test is mutation-checked: remove the window predicate and it must fail. A test that
    passes either way is what feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) #17725's first attempt at this class produced.

Related

Follow-up to #16818, found by review on PR #17725. Does not block that PR.

Activity

  1. mrveiss commented on Sep 28, 2026

    @mrveiss
    OwnerAuthor

    Premise check — not verifiable against main: this reviews code that has not landed. Read-only at origin/main ab05ba3aef.

    The file this issue is about does not exist on main. git ls-tree -r origin/main finds no workspace_lease_sweep* and no workspace_disposal* anywhere in the tree. Both this issue and its sibling are reviews of #16818's implementation in PR #17725, which is reviewed and in the CI queue rather than merged.

    That has three consequences worth recording before anyone schedules it:

    1. A premise check against main cannot confirm or refute it. The evidence for it lives at feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) #17725's head. Any verdict I posted against main would be a statement about a tree that does not contain the subject.
    2. If feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) #17725 merges as it stands, this becomes a live defect on main the moment it lands — not new work discovered later, but a known finding arriving with the code. Folding it into feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) #17725 before merge costs one review round; landing it and fixing it afterwards costs a second PR and leaves main briefly carrying a defect that was known in advance.
    3. The cheaper sequencing is therefore to resolve it inside feat(llc): make the workspace lease real — acquire, release, reclaim, and dispose on evidence (#16818) #17725, and to keep this issue open only if the owner prefers the fix to land separately. That is a scheduling choice, not an engineering one, and it should be made before the merge rather than after.

    Nothing here disputes the finding. It is a statement about where it can be verified and when it is cheapest to act on.

  2. added 2 commits that reference this issue on Sep 29, 2026
    f105d4f
    9fc8995
  3. added a commit that references this issue on Oct 4, 2026
    919c49e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions