Skip to content

Fail stale applying memory proposals - #283

Open
caioribeiroclw-pixel wants to merge 1 commit into
BetterDB-inc:masterfrom
caioribeiroclw-pixel:caio/stale-memory-apply-reaper
Open

caioribeiroclw-pixel wants to merge 1 commit into
BetterDB-inc:masterfrom
caioribeiroclw-pixel:caio/stale-memory-apply-reaper

Conversation

@caioribeiroclw-pixel

Copy link
Copy Markdown

Summary

  • add a storage-level sweep for memory proposals stuck in applying after the apply grace window
  • mark stale applies as failed with a stale_apply applied_result and audit event
  • cover the memory and SQLite adapters plus service-level stale/applying behavior

Why

Issue #277 notes that a crash after approved -> applying can leave a visible applying row forever. This keeps that safe failure mode during the crash window, but lets the normal expiration cron surface old stuck applies as failed instead of requiring manual inspection.

Checks

  • SKIP_DOCKER_SETUP=true pnpm test -- --runTestsByPath src/storage/adapters/__tests__/memory-proposals.spec.ts src/storage/adapters/__tests__/memory-proposals-sqlite.spec.ts ✅
  • git diff --check ✅

I also tried the service spec path, but this checkout currently fails before running that suite because ../../proprietary/memory-proposals/memory-apply.dispatcher.ts cannot resolve @betterdb/agent-memory; leaving the service-level tests in this PR so CI with the normal workspace setup can exercise them.

Closes #277

@github-actions

Copy link
Copy Markdown

Thank you for your contribution! Before we can merge this PR, you need to sign our Contributor License Agreement.

To sign, please comment below with:

I have read the CLA Document and I hereby sign the CLA


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@KIvanow

KIvanow commented Jul 7, 2026 •

Copy link
Copy Markdown
Member

Thanks for the contribution! Clean fix for a real edge case — the stale-apply sweep is well-scoped, all three adapters are consistent, and the test coverage is solid. Appreciate you closing out #277.

@caioribeiroclw-pixel Before we can merge, you'll need to sign our CLA. Just post a comment on this PR with exactly:

I have read the CLA Document and I hereby sign the CLA

Details here: CLA Document

@KIvanow KIvanow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamby77

jamby77 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

@caioribeiroclw-pixel — this is approved and ready, but it is blocked on one
thing only: the CLA is unsigned, so CLAAssistant is the sole failing check.

To unblock it, comment exactly this on this PR:

I have read the CLA Document and I hereby sign the CLA

That is the only step. Nobody else can sign on your behalf, which is why this
has been sitting since June despite KIvanow's approval on 07-07.

Why I am asking now

I built the same fix for #277 in #416 before noticing this PR existed. That was
my miss — I planned from the issue text and did not check for an open PR. Rather
than have your approved work superseded, the plan is to land this first and
strip the overlapping half out of #416, which keeps its own separate work on
#276 (the duplicate-pending race) and a set of migration fixes.

So this is not a courtesy ping — #416 is waiting on it.

One thing worth a look when you get to it

The two implementations time staleness differently. This PR uses reviewed_at;
mine added an applying_at column stamped at the approved -> applying claim.

Yours is simpler and needs no migration, and the two are equivalent today
because approve and apply happen inside one request. The reason I went the other
way is that they stop being equivalent the moment that changes — if approval is
ever decoupled from apply (a queue, a scheduled apply), a sweep measured from
reviewed_at starts failing work that is still running.

Not a blocker, and not a reason to change this PR. Worth recording in the code
or the issue so the assumption is visible if anyone touches that path later.

Also note this branch last ran CI in June. It reports mergeable against current
master, but a rebase before merge would be worth it — a few things in that area
have moved since, including migration ordering on existing databases.

@jamby77

jamby77 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Heads up: #416 was merged as-is, which includes the stale-applying sweep this PR implements, so this work has been superseded. Sorry — the CLA never cleared and we chose not to hold the #276 duplicate-pending fix behind it any longer.

Recording the one substantive difference for anyone reading later: this PR times staleness from reviewed_at, #416 from an applying_at column stamped at the approved -> applying claim. Equivalent while approve and apply happen in one request; if that is ever decoupled, reviewed_at would start failing work that is still running.

Thanks for the contribution, and sorry it landed this way.

This branch has not been deployed

No deployments
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.

memory forget: stuck 'applying' proposals never expire or recover

3 participants