Skip to content

fix(act): don't quarantine a stream while the database is failing (ACT-1592) - #1593

Merged
Rotorsoft merged 2 commits into
masterfrom
act-1592-store-failure-retry-budget
Aug 29, 2026
Merged

fix(act): don't quarantine a stream while the database is failing (ACT-1592)#1593
Rotorsoft merged 2 commits into
masterfrom
act-1592-store-failure-retry-budget

Conversation

@Rotorsoft

@Rotorsoft Rotorsoft commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Closes #1592.

A database that fails for a second could permanently stop a stream from processing, and this stops that.

The background

Every stream Act reacts to carries a strike count. It goes up when work is handed to a worker, and back to zero when that worker reports finishing. Run out of strikes and the stream is quarantined: it stops, stays stopped, and waits for a human to clear it. That rule earns its keep, because the alternative is a broken handler firing the same webhook every few milliseconds until somebody notices.

What was going wrong

The strike goes on the board when work is handed out, before the handler has done anything, and it only comes off when the worker successfully reports back. Reporting back means writing to the database, and the database can refuse. When it did, the whole round was abandoned and the strike stayed on the board even though the handler had run fine and finished its job.

Four of those in a row and the stream was quarantined. The handler never failed once.

There is a second, unrelated situation that produces the identical symptom, which is a handler so slow it loses its turn every single round. So Act assumed that is what happened and printed that problem's advice: your handler needs a longer lease, go increase it. For the person who reported this, that advice was useless. Their handlers finished in a few milliseconds against a ten second lease, and their commands had all committed. The real cause was a few lines up in the log as a "database is locked" message, from a second connection to the SQLite file. That happens for a moment every time a watching dev server restarts over itself, and #1578 already established that it is normal and that turning up the database's patience setting is not the fix.

Worth noting: Act already knew whose fault it was. The same error handler that abandoned the round is the one that reports the failure to the circuit breaker as the store's. Two parts of the system held opposite opinions about who spent the strike and nothing had ever reconciled them.

What shipped

The quarantine now stands down whenever the database has failed since the last round that completed. The circuit breaker was already counting exactly that, so nothing new had to be recorded anywhere. It reads that instead of keeping its own tally.

Waiting costs nothing. A stream that is genuinely stuck stays stuck. The next round that completes either wipes the count, because the work finally went through, or leaves it standing for the quarantine to act on once the database is healthy.

The message also stops over-claiming. It used to assert that the lease was lost on every attempt as if that were the only possible cause; now it says the budget was spent with no handler error and then gives the lease explanation as the reading, which is accurate again now that a failing database cannot reach that code.

The first attempt, and why it was dropped

The first version of this took the ticket's suggestion literally and kept a running tally, per stream, of strikes the database had thrown rather than the handler, subtracting it from every decision that reads the count. It was exact and it was about a hundred lines, with state to reason about across rounds and restarts. Roger read it and called it overcomplicated, which it was. The simpler move is to notice that the quarantine makes a claim it cannot support at that moment, and to stop making it, rather than build machinery to make the claim true again.

Two other designs were looked at and dropped. Undoing the strike when the store fails reads well until you notice where that code sits: you are there because the database just refused a write, and the plan is to ask it for another one. Moving the strike from the moment work is handed out to the moment it is reported back is the more honest repair, and it changes a contract every adapter implements in order to fix an accounting detail. It also gives up something real, because the strike is recorded up front precisely so it survives a worker that dies mid-handler and reports nothing at all, which is what stops a stream whose events kill workers from killing workers forever.

What it costs

One case is knowingly left on the wrong side. If the database eats a few strikes and then a handler starts failing for real, that handler is quarantined sooner than its full budget allows, because it inherits a count it did not earn. Its own error is logged either way, so the operator sees the real problem, and unblocking resets the count so the next round gives it the full budget. Being early on a genuinely broken handler is a far smaller wrong than quarantining a healthy one, and closing it is exactly the hundred lines that were not worth writing.

The signal is per process and shared across lanes in that process. A database failing for one lane briefly stands the quarantine down for the others too. That errs toward not quarantining, which is the safe direction.

This does not rescue anything already quarantined by this bug. Those streams still need app.unblock(...).

Recommendation

Nothing for operators to change. If you have streams quarantined with a message about lost leases and handlers that plainly are not slow, check your logs for database errors around that time, then unblock them.

Test plan

  • pnpm test — 236 files, 3610 passed, 54 skipped
  • Coverage: 100% statements / 100% branches / 100% functions / 100% lines.
  • pnpm typecheck
  • pnpm lint
  • pnpm build
  • pnpm -F docs check:snippets
  • New libs/act/test/store-failure-budget.spec.ts, both cases checked against master first: without the change the healthy stream is quarantined on its fifth round
  • The existing lease-loss tests (non-retryable.spec.ts, act-pg lease-loss.spec.ts) still pass, which is what proves the quarantine was narrowed rather than switched off
  • CI green
  • Review

Stability charter impact

None. The change is one new condition on an internal function in libs/act/src/internal/drain-cycle.ts and one new read-only getter on the internal CircuitBreaker, both of which the charter puts out of scope. No builder, IAct, port, or lifecycle-event surface changed. Behavior moves in one direction only: a stream that would have been quarantined during a database failure no longer is.

rfc-gate: exempt — the stability snapshot grew only because it captures source text, and what grew is an internal drain-cycle parameter, an internal breaker getter, and their comments. No public export, builder method, port method, lifecycle event, or field on an exported type was added.

Docs

concurrency-model.md gains a short section on the database-spent count, including the case left open; error-handling.md says a failing store is deliberately not a fourth way to be quarantined; extension-points.md notes adapters owe nothing here; behavior-contracts.md gets a row pointing at the new test; the CLAUDE.md one-liner is updated; and book/act-1592-the-database-spent-the-budget.md carries the story, including the ledger design that was dropped.

Follow-ups

None parked.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JvPC284DA2EqnuyRdsVfNh

…y budget

`claim` persists `retry = retry + 1` before any handler runs, and only a
confirmed write-back resets it. A cycle that throws on `fetch`, `block` or
`ack` therefore walked the counter up with no attempt behind it — four
transient `SQLITE_BUSY` rounds blocked the stream and prescribed a bigger
`leaseMillis` for a handler that never failed.

The drain now keeps a per-lane ledger of claims charged to the store. Every
claim is charged provisionally; a cycle that finalizes converts its claim into
a handler attempt, a cycle that throws leaves it charged, and the next claim
discounts what is still charged out of the `retry` the block decisions read.
The discounted lease travels through the claim-time budget check, the
dispatcher's block decision, and the counter a due-marked ack writes back, so
a refund becomes durable the moment the store confirms anything for the
stream. Handler failures still block after exactly `maxRetries` attempts, and
a lease genuinely lost every round still blocks at claim time.

Closes #1592
@Rotorsoft Rotorsoft added bug Something isn't working priority:high Important features/bugs risk:medium API changes, new features type:fix Bug fix labels Aug 29, 2026
@Rotorsoft Rotorsoft self-assigned this Aug 29, 2026
Replaces the per-stream ledger from the previous commit with the check the
ticket suggested: don't quarantine a stream while the store is failing.

`claim` writes the retry count up before any handler runs, and only a pass
that reaches its ack writes it back down, so a pass that dies on a store call
leaves a count no handler earned. A few of those look exactly like a lease
lost every round, which is what the claim-time block exists to catch, so a
healthy stream was quarantined over a transient database lock and told to
raise leaseMillis.

The circuit breaker already counts store failures since the last completed
pass, so the block reads that instead of keeping its own tally. Nothing is
lost by waiting: a genuinely stuck stream stays stuck, and the next completed
pass either resets the count or leaves it standing for the block to act on.

One case is knowingly left: a handler that starts failing right after a store
outage is blocked earlier than its full budget, since it inherits a raised
count. Its own error is logged, and unblock resets the count.

Closes #1592
@Rotorsoft Rotorsoft changed the title fix(act): don't charge a store failure to the handler's retry budget (ACT-1592) fix(act): don't quarantine a stream while the database is failing (ACT-1592) Aug 29, 2026
@Rotorsoft
Rotorsoft merged commit 25f308b into master Aug 29, 2026
18 checks passed
@Rotorsoft
Rotorsoft deleted the act-1592-store-failure-retry-budget branch August 29, 2026 21:45
@github-project-automation github-project-automation Bot moved this from Todo to Done in Act Roadmap 2026 Aug 29, 2026
github-actions Bot pushed a commit that referenced this pull request Aug 29, 2026
# [@rotorsoft/act-v1.31.14](https://github.com/Rotorsoft/act-root/compare/@rotorsoft/act-v1.31.13...@rotorsoft/act-v1.31.14) (2026-08-29)

### Bug Fixes

* **act:** don't quarantine a stream while the database is failing (ACT-1592) ([#1593](#1593)) ([25f308b](25f308b)), closes [#1592](#1592)
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version @rotorsoft/act-v1.31.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions Bot pushed a commit that referenced this pull request Aug 29, 2026
# [@rotorsoft/act-tck-v1.36.14](https://github.com/Rotorsoft/act-root/compare/@rotorsoft/act-tck-v1.36.13...@rotorsoft/act-tck-v1.36.14) (2026-08-29)

### Bug Fixes

* **act:** don't quarantine a stream while the database is failing (ACT-1592) ([#1593](#1593)) ([25f308b](25f308b)), closes [#1592](#1592)
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version @rotorsoft/act-tck-v1.36.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

bug Something isn't working priority:high Important features/bugs released risk:medium API changes, new features type:fix Bug fix

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

A transient store failure on ack spends the handler retry budget and blocks the stream

1 participant