feat(notify,core,cli): the two notify tables get a sweep, and the inbox window is the app's to name - #383
Conversation
…ox window is the app's to name `x_notify_inbox` was the one framework table nothing swept, and `x_notify_deliveries` was the second — the issue said the ledger already had a target and `retentionTargets` returned exactly three. Both tables' DDL is applied by every boot (`framework-schema.ts`), so both grew without bound in every deployment the framework produces. WHY THE INBOX NEEDED A CONFIG KEY AND THE OTHERS DID NOT. Every other framework table holds bookkeeping whose job ENDS — an idempotency key, a rate-limit bucket, an auth challenge, a delivery claim. An inbox row is a message a person has not read yet, so when it disappears is a product decision, which is axiom 8. `notify.inboxReadRetentionMs` and `notify.inboxUnreadRetentionMs` are therefore the app's, **both absent by default**, and absent means never swept. Two windows rather than one because the objection is only about UNREAD messages: read notices gone in a month with unread ones kept forever is what an app actually wants, and it is only expressible if the two are separate. A read row ages from `read_at` and an unread one from `created_at` — ageing a read row from `created_at` would delete a notification the moment the recipient opened an old one. THE SEAM IS READ PER ATTEMPT, NOT HANDED OVER. `setNotifyStores` is an app's boot line and runs when the app's modules import, which is AFTER `installRetentionSweep`. So `retention.ts` asks `notifyStores()` on each sweep and answers 0 for a memory store or none at all — the same shape as `purgeAuthLimits()`, and the reason `authTarget` is not in `RetentionStores` either. `purgeBefore` and `purgeExpired` are on the Postgres stores' own wider types (`PgInboxStore`, `PgDeliveryLedger`), never on the seam every implementation must satisfy: adding a method there breaks an app that wrote its own, and a heap map bounded by process life has nothing to delete. `PostgresIdempotencyStore` already has exactly this shape. Both statements carry `returning`. A bare `delete` answers no rows through `PgExecutor`, so the count would be zero forever while the delete succeeded — a sweep that reads in a log exactly like one that is not wired at all. `DevServices` gains `root`: `loadInboxRetention` reads the app's own `app.config.ts` and `startServices` holds no `AppConfig`, which is the same reason `configureAuthLimiters` takes a factory. Fixes #363 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The live suite asserted the three-target list by name, so adding two targets turned it red in CI — which is the assertion working. Updated, and extended: `dev-purge.test.ts` proves the wiring with stubs and can prove nothing about the SQL, and a purge whose statement never executed is exactly the state both notify tables shipped in. The windows travel the REAL path — `boot()` writes an `app.config.ts` into the scratch root and `loadInboxRetention` reads it — because handing them to `installRetentionSweep` directly would prove the sweep and not the wiring, and the wiring is the half a config key loses. One read row deleted and the old UNREAD row kept, because no unread window was configured: the default, and the axiom-8 promise this key exists to keep. `read_at` is stamped with SQL rather than through `inbox.markRead`. That call binds a uuid ARRAY and Bun does not encode a JS array as a Postgres array parameter at all — issue #384, found by this test, which also takes down `SQL_CLAIM` (the worker's claim loop) and `SQL_OUTBOX_RELEASE`. Fixed in its own change; using `markRead` here would make a retention test fail for a reason that is not retention.
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 17 seconds for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 75 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
Comment |
|
This PR's live test found a critical pre-existing defect, filed as #384.
Three shipped statements bind an array through that path, and all three fail:
Why nothing caught it: It is fixed in its own change, not here. This PR is about retention; #384 is about parameter encoding across two packages, and folding a critical fix into a feature PR makes both harder to review and to revert. The live test stamps The updated live suite passes against real Postgres 17 — 4 of 4, with the notify sweep really removing rows (1 delivery, 1 inbox), which is what proves the statements execute at all. |
|
@coderabbitai review |
|
|
Hi @sebyx07 - I see you requested a review from @coderabbitai. I'm the maintainer automation bot (developerz.ai[bot]), not CodeRabbit. I'll let CodeRabbit's review proceed. Once CI is green and reviews are in, feel free to tag me for any follow-up triage needs. 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
|
Looks good — CI green, no risk signals, and this is a routine feature addition. Ready for maintainer merge when ready. 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
1 participant
Closes #363.
Two premises in the issue were false, and they changed the shape of the work
Checked before starting; both recorded on the issue.
x_notify_deliverieswas NOT swept. The issue's §"What was decided" said it shipped as a 4thPurgeTarget.retentionTargetsreturned exactly three —x_idempotency,x_rate_limit,x_auth— anddev-purge.ts's own header said "the three framework tables". So both notify tables were unswept.createPgDeliveryLedgernorcreatePgInboxStorehad a caller outsidepackages/notify/.The second is not a hole —
framework-schema.ts:100says so deliberately: the DDL is applied by every boot "whether or not this boot callssetNotifyStores, for the same reason as every row above it — that call is an APP's boot line and runs after this one." Which is what fixes the design of the sweep: it cannot hold the stores, only ask what is installed now.Why the inbox needed a config key and the others did not
Every other framework table holds bookkeeping whose job ends — an idempotency key, a rate-limit bucket, an auth challenge, a delivery claim. A sweep is unambiguously right. An inbox row is a message a person has not read yet, so when it disappears is a product decision, not a framework one. Axiom 8.
notify.inboxReadRetentionMsnotify.inboxUnreadRetentionMsTwo windows, not one, because the axiom-8 objection is specifically about unread messages. Read notices gone in a month with unread ones kept forever is what an app actually wants, and it is only expressible if the two are separate:
A read row ages from
read_at, an unread one fromcreated_at— ageing a read row fromcreated_atwould delete a notification the moment the recipient opened an old one. One statement, each half inert when its cutoff isnull.Milliseconds and not a
DurationInput: that type is@ultimat3/jobs(tier 3) andAppConfigis tier 0 — the same reasoncache.defaultTtlMsis spelled this way. Zero is refused rather than read as "immediately", because a sweep at age 0 is an inbox that silently receives nothing.Both halves in one change, so neither key is declared-and-never-wired
bun run scripts/config-readers.ts— 29 leaf keys, every one read. The reader isdev-notify-retention.ts, the sibling ofloadSignInPathandloadCacheTiers;startServicesholds noAppConfig, which is the same reasonconfigureAuthLimiterstakes a factory.DevServicesgainsrootfor it, carried rather than re-derived fromstateDir— adirnamethat silently disagreed withjoin(root, '.x')is a path bug nothing would catch.The purge lives on the Postgres stores' own types
PgInboxStore.purgeBeforeandPgDeliveryLedger.purgeExpired, never onInboxStore/DeliveryLedger. Adding a method to the seam every implementation must satisfy is a breaking change for an app that wrote its own, and a heap map bounded by process life has nothing to delete.PostgresIdempotencyStorealready has exactly this shape,windowMsincluded.retention.tsreads the seam per attempt and answers 0 for a memory store or none at all — the same shape aspurgeAuthLimits(), and the reasonauthTargetis not inRetentionStoreseither. Atypeof store.purgeBefore === 'function'check over a declared widening, not duck typing.Both statements carry
returningA bare
deleteanswers no rows throughPgExecutor, so a count read off it is zero forever while the delete succeeds — a sweep that reads in a log exactly like one that is not wired at all.SQL_IDEMPOTENCY_PURGEis spliced the same way for the same reason.The delivery window is not a config key
It is
createPgDeliveryLedger({ executor, windowMs }), stated beside the statement that reads it, default 24 h. Never shorter than the app's idempotency window — a job replayed inside that window against a claim already purged claims cleanly and sends the notification a second time. Passingidempotency.windowMsmakes that impossible by construction rather than by two defaults that happen to agree.Tests
retention.test.ts— all four states the seam can be found in. Mutation-proven: relaxing the capability check to always-true turns 3 of 6 red.inbox-pg.test.ts— each half of the statement reads the column its window is about;returning idpresent; an absent window bindsnull; neither window set issues no statement at all (the default state of every app, on an hourly sweep).ledger-pg.test.ts— the cutoff is the caller's clock minus the ledger's own window; the window is readable so a caller can copy it; a non-positive window is refused at construction.dev-purge.test.ts— the target list asserted by name, never counted:x_notify_inboxbecame the unswept table by being absent from a list nothing asserted, and a length check cannot say which one went missing. Plus each window arriving as its own cutoff, and an unset window arriving absent rather than as aNaNdate every row is older than.dev-notify-retention.test.ts— a realapp.config.tson disk, including the four shapes that must read as "never swept".config.test.ts— the screened-key list asserted againstNotifyConfig's own keys, so a third window with no screen is a red test.Docs corrected
packages/notify/CLAUDE.mdclaimed the boot "does not installx_notify_deliveriesorx_notify_inbox" — true when written, false since. Replaced with the retention table.wiki/Configuration.mdgains thenotifysection;packages/notify/README.mdgains a Retention section and itssetNotifyStoresexample now passeswindowMs.Verified
bun run verify— 14 of 20 passed, 6 skipped (drift, contract-diff, budgets, seo, i18n, policy), the documented root baselinebun run scripts/reference-app-gate.ts— every pin holds:examples/dummy18/20 (2 pinned),dummy/social-media-clone18/20 (2 pinned)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.