Skip to content

Commit f1850d8

Browse files
hotlongclaude
andauthored
fix(services): markAllRead 清空整个收件箱,不再只清列表窗口的 200 条 (#6436) (#6449)
`POST /api/v1/notifications/read/all` 声明的是「mark **every** currently-unread inbox message as read」,实现却扫 `listInbox(userId, { read: false, limit: 200 })` —— 列表的一页,而 200 正是那个列表的硬上限,于是一次调用最多翻 200 条收据。 真实栈实测(sqlite-wasm + ObjectQL + service-messaging + hono + dispatcher), 单用户 260 条未读: | 请求 | 修复前 | 修复后 | |:---|---:|---:| | `POST /notifications/read/all` | `readCount: 200` | `readCount: 260` | | 紧接着 `GET /notifications` | `unreadCount: 60` | `unreadCount: 0` | **#6363 不是成因,它掀掉了掩盖的布**:`unreadCount` 还按窗口数时,截断是自洽 且隐身的(清 200、再轮询、窗口里一条不剩、角标 0);角标变成真总数之后,同一对 请求自己把矛盾说了出来,严重度也随之从「静默漏清」升为「点了全部已读、界面仍 显示未读」。 **同一缺陷更锋利的另一面,一并修掉**:那个窗口是对**全部**行按 `created_at desc` 取的,`read` 过滤在截断**之后**于内存中施加。所以最新 200 条已读的信箱, 交给清扫的是一个**空** id 列表 —— 无论后面压着多少更旧的未读,它一条都不标。 这也正是「循环翻页直到取空」不成立的原因:它恰好在那空的第一页退出。 **现在的做法**:读未读**集合**而不是列表的一页,且不论信箱多大都是**固定两次 读** —— 与 #6363 的 `countUnreadTotal` 为角标所发的那一次单列、无窗口投影同形, 再与 `listInbox` 本就无界读取的收据脊连接。没有循环,没有需要设上界的页数;向 数据层要的东西,不超出角标轮询在每个打满的页上已经要的。写入仍是每条未读通知 一条收据 —— 那是收据模型本身(ADR-0030);`markRead` 的 check-then-act upsert、 unique 冲突收敛与「尚无收据行」的插入面**均未改动**(路线 B 的重设计条件因此未 触发)。 `readCount` 现在报的是**本次调用真正翻成 `read` 的去重通知条数**(此前报的是 「最新 200 行里未读的那些」)。两个推论:一条通知被多行收件箱行物化时只计一次; 没有 `notification_id` 的收件箱行被跳过而不是计入 —— 读态以事件 id 为键,旧代码 为它们写的那条(以收件箱**行** id 为键的)收据,连接永远读不回来。该行恒为未读的 结构性缺口另行记为 #6448(休眠:唯一 ingress `emit()` 必带事件 id)。 不变:列表窗口(默认 50、上限 200、最新在前)、`unreadCount`、`markRead`,以及 小于旧窗口的信箱 —— 它做的写入与从前完全一致,一次也不多。 Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD Co-authored-by: Claude <noreply@anthropic.com>
1 parent d2b97c3 commit f1850d8

4 files changed

Lines changed: 414 additions & 24 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@objectstack/service-messaging": patch
3+
---
4+
5+
fix(services): `markAllRead` clears the WHOLE inbox, not one 200-row window (#6436)
6+
7+
`POST /api/v1/notifications/read/all` is published as "mark **every**
8+
currently-unread inbox message as read". It swept
9+
`listInbox(userId, { read: false, limit: 200 })` — one page of the LIST, and
10+
`200` is that list's hard cap — so it cleared at most 200 receipts per call.
11+
12+
Measured over the real stack (sqlite-wasm + ObjectQL + service-messaging + hono
13+
+ dispatcher), one user, 260 unread:
14+
15+
| request | before | after |
16+
|:---|---:|---:|
17+
| `POST /notifications/read/all` | `readCount: 200` | `readCount: 260` |
18+
| `GET /notifications` (same user, next) | `unreadCount: 60` | `unreadCount: 0` |
19+
20+
**#6363 did not cause this — it removed the cover.** While `unreadCount` was
21+
itself window-scoped the shortfall was self-consistent and invisible: clear 200,
22+
poll, see a window with nothing unread in it, badge 0. Now that the badge is a
23+
true total, the same request pair states the contradiction out loud, which also
24+
raises the severity — a user presses "mark all read" and the badge stays lit.
25+
26+
**A second, sharper face of the same defect, fixed with it.** That window was
27+
`created_at desc` over ALL rows, with the `read` filter applied in memory AFTER
28+
the truncation. An inbox whose newest 200 were already read therefore handed the
29+
sweep an EMPTY id list and marked **nothing at all**, however much older unread
30+
sat behind it. That is also why "loop the pages until one comes back empty" is
31+
not the fix: it exits on exactly that empty first page.
32+
33+
**What it does now.** It reads the unread SET rather than a page of the list, in
34+
a FIXED two reads whatever the inbox size — the same one-column, unwindowed
35+
projection of `sys_inbox_message` that #6363's `countUnreadTotal` already issues
36+
to answer the badge, joined against the receipt spine `listInbox` already reads
37+
unbounded. No loop and no page count to bound; nothing is asked of the data
38+
layer that the bell's poll does not already ask on every saturated page. The
39+
write stays one receipt per unread notification — that is the receipt model
40+
itself (ADR-0030) — and `markRead`'s check-then-act upsert, its unique-conflict
41+
convergence and its "no receipt row yet" insert are untouched.
42+
43+
`readCount` now reports the number of **distinct notifications this call flipped
44+
to `read`** (it reported "the unread ones inside the newest 200 rows"). Two
45+
consequences: a notification materialized by several inbox rows counts once, and
46+
an inbox row carrying no `notification_id` is skipped rather than counted —
47+
read-state is keyed by the event id, so the receipt the old code wrote for those
48+
(keyed by the inbox ROW id) was one the join could never read back.
49+
50+
Unchanged: the list window (default 50, cap 200, newest first), `unreadCount`,
51+
`markRead`, and an inbox smaller than the old window — which does the same
52+
writes it always did, and no extra ones.

‎packages/runtime/src/notifications.hono.integration.test.ts‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,14 @@ describe('in-app notifications over a real hono server (integration, #3362)', ()
104104
}
105105
}, 30_000);
106106

107-
const authed = (path: string, init?: RequestInit) =>
107+
const as = (user: string, path: string, init?: RequestInit) =>
108108
fetch(`${baseUrl}${path}`, {
109109
...init,
110-
headers: { 'x-test-user': TEST_USER, 'content-type': 'application/json', ...(init?.headers ?? {}) },
110+
headers: { 'x-test-user': user, 'content-type': 'application/json', ...(init?.headers ?? {}) },
111111
});
112112

113+
const authed = (path: string, init?: RequestInit) => as(TEST_USER, path, init);
114+
113115
it('resolves the notification service in discovery (declared === enforced)', async () => {
114116
const res = await fetch(`${baseUrl}/api/v1/discovery`);
115117
expect(res.status).toBe(200);
@@ -173,4 +175,54 @@ describe('in-app notifications over a real hono server (integration, #3362)', ()
173175
expect(receipts.length).toBe(2);
174176
expect(receipts.every((r: any) => r.state === 'read')).toBe(true);
175177
});
178+
179+
it('[#6436] mark-all-read clears an inbox LARGER than the list window — no readCount/unreadCount contradiction', async () => {
180+
// The issue, over the wire, on the real stack. `markAllRead` swept one page
181+
// of the list (`limit: 200`, that list's hard cap), so a user with more
182+
// unread than the cap pressed "mark all read" and the badge did not clear:
183+
//
184+
// POST /api/v1/notifications/read/all → { readCount: 200 }
185+
// GET /api/v1/notifications → { unreadCount: 150 }
186+
//
187+
// #6363 did not cause that — it removed the cover. While `unreadCount` was
188+
// window-scoped the shortfall was self-consistent and invisible; once the
189+
// badge became a true total the same request pair states it out loud, which
190+
// is why this pin lives at the wire and not only under the service.
191+
//
192+
// Rows are seeded through the data engine rather than 260 `emit()` calls:
193+
// this asserts the READ/SWEEP path over HTTP, and the delivery path is
194+
// already covered by the test above.
195+
const BULK_USER = 'usr_notif_bulk';
196+
const TOTAL = 260; // > the list's hard cap of 200
197+
const data = kernel.getService<IDataEngine>('data');
198+
for (let i = 0; i < TOTAL; i++) {
199+
await data.insert('sys_inbox_message', {
200+
user_id: BULK_USER,
201+
notification_id: `bulk_n${String(i).padStart(3, '0')}`,
202+
topic: 'deal.won',
203+
title: `Bulk ${i}`,
204+
body_md: 'body',
205+
severity: 'info',
206+
created_at: `2026-02-01T00:00:00.${String(i).padStart(3, '0')}Z`,
207+
});
208+
}
209+
210+
const before = await (await as(BULK_USER, '/api/v1/notifications')).json();
211+
expect(before.data.unreadCount).toBe(TOTAL); // the true total (#6363)
212+
expect(before.data.notifications).toHaveLength(50); // the list is still one window
213+
214+
const readAll = await (await as(BULK_USER, '/api/v1/notifications/read/all', { method: 'POST' })).json();
215+
expect(readAll.data).toMatchObject({ success: true, readCount: TOTAL }); // was 200
216+
217+
const after = await (await as(BULK_USER, '/api/v1/notifications')).json();
218+
expect(after.data.unreadCount).toBe(0); // was 60
219+
expect(after.data.notifications.every((n: any) => n.read === true)).toBe(true);
220+
221+
// Persisted, not computed: one `read` receipt per notification.
222+
const receipts = await data.find('sys_notification_receipt', {
223+
where: { user_id: BULK_USER, channel: 'inbox' },
224+
});
225+
expect(receipts.length).toBe(TOTAL);
226+
expect(receipts.every((r: any) => r.state === 'read')).toBe(true);
227+
}, 120_000);
176228
});

‎packages/services/service-messaging/src/messaging-service.test.ts‎

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -793,3 +793,192 @@ describe('[#6363] listInbox — unreadCount is the TOTAL unread, not the fetched
793793
expect((await svc.listInbox('u1', { limit: 120 })).notifications).toHaveLength(120);
794794
});
795795
});
796+
797+
/**
798+
* [#6436] `markAllRead` swept `listInbox(userId, { read: false, limit: 200 })`
799+
* — one page of the LIST — and `200` is that list's hard cap, so the route
800+
* documented as "mark **every** currently-unread inbox message as read"
801+
* cleared at most 200 receipts per call.
802+
*
803+
* #6363 did not introduce this; it removed the cover. While `unreadCount` was
804+
* counted over the window the truncation was self-consistent and invisible
805+
* (clear 200, poll, see a window with nothing unread in it, badge 0). Now that
806+
* the badge is the true total, one response pair states the contradiction on
807+
* its own: `POST /read/all → { readCount: 200 }` then
808+
* `GET /notifications → { unreadCount: 150 }`.
809+
*
810+
* Route C — redefine "all" as "the current window" — was excluded by the
811+
* maintainer's #6363 Option A ruling (make the declaration true). The sweep now
812+
* reads the unread SET directly instead of a page of the list.
813+
*/
814+
describe('[#6436] markAllRead — sweeps the whole inbox, not one 200-row window', () => {
815+
const logger = silentLogger();
816+
817+
it("clears an inbox holding more unread than the list's hard cap (the issue's 350)", async () => {
818+
const engine = inboxEngine({ inbox: seedInbox('u1', 350) });
819+
const svc = new MessagingService({ logger, getData: () => engine });
820+
821+
const res = await svc.markAllRead('u1');
822+
// Before: `readCount: 200`, and 150 messages still unread behind a
823+
// badge that — since #6363 — reported them correctly.
824+
expect(res).toEqual({ success: true, readCount: 350 });
825+
expect((await svc.listInbox('u1')).unreadCount).toBe(0);
826+
827+
// Persisted read-state, not a view-layer computation: one receipt per
828+
// notification, every one of them `read`.
829+
const receipts = engine.store.sys_notification_receipt;
830+
expect(receipts).toHaveLength(350);
831+
expect(receipts.every((r: any) => r.state === 'read')).toBe(true);
832+
});
833+
834+
it('marks the older unread even when the newest 200 are already read', async () => {
835+
// The sharper face of the same defect, and the reason "loop `listInbox`
836+
// until it comes back empty" is not merely costly but WRONG: the window
837+
// is `created_at desc` over ALL rows and the `read` filter is applied
838+
// in memory AFTER the truncation. An inbox whose newest 200 are read
839+
// therefore handed the sweep an EMPTY id list — it marked nothing at
840+
// all, however much older unread sat behind it, and a paging loop would
841+
// have exited on that same empty first page. Reading the unread SET
842+
// makes a message's position in the inbox stop mattering.
843+
const engine = inboxEngine({
844+
inbox: seedInbox('u1', 350),
845+
// m151…m350 are the NEWEST 200 (created_at .150 … .349).
846+
receipts: Array.from({ length: 200 }, (_, i) => readReceipt('u1', i + 151)),
847+
});
848+
const svc = new MessagingService({ logger, getData: () => engine });
849+
expect((await svc.listInbox('u1')).unreadCount).toBe(150);
850+
851+
expect((await svc.markAllRead('u1')).readCount).toBe(150); // before: 0
852+
expect((await svc.listInbox('u1')).unreadCount).toBe(0);
853+
});
854+
855+
it('a small inbox behaves exactly as before — only the unread flip', async () => {
856+
const engine = inboxEngine({
857+
inbox: seedInbox('u1', 10),
858+
receipts: [readReceipt('u1', 1), readReceipt('u1', 2)],
859+
});
860+
const before = engine.store.sys_notification_receipt.map((r: any) => ({ ...r }));
861+
const svc = new MessagingService({ logger, getData: () => engine });
862+
863+
expect(await svc.markAllRead('u1')).toEqual({ success: true, readCount: 8 });
864+
expect((await svc.listInbox('u1')).unreadCount).toBe(0);
865+
866+
// The two already-read receipts are not re-stamped: an inbox smaller
867+
// than the old window is the case that was never broken, and it must
868+
// not start doing extra writes to prove it.
869+
const after = engine.store.sys_notification_receipt;
870+
expect(after).toHaveLength(10);
871+
expect(after.slice(0, 2)).toEqual(before);
872+
});
873+
874+
it('costs a fixed two reads however large the inbox is — no paging loop', async () => {
875+
for (const n of [10, 350]) {
876+
const engine = inboxEngine({ inbox: seedInbox('u1', n) });
877+
const calls = recordFinds(engine);
878+
const svc = new MessagingService({ logger, getData: () => engine });
879+
880+
await svc.markAllRead('u1');
881+
882+
expect(calls, `inbox of ${n}`).toHaveLength(2);
883+
const inboxRead = calls.find((c) => c.object === 'sys_inbox_message')!;
884+
// Unwindowed, unordered and one column wide — the same projection
885+
// #6363's `countUnreadTotal` already reads to answer the badge, so
886+
// the sweep asks the data layer for nothing the bell poll does not
887+
// ask it on every saturated page.
888+
expect(inboxRead.query.where).toEqual({ user_id: 'u1' });
889+
expect(inboxRead.query.fields).toEqual(['notification_id']);
890+
expect(inboxRead.query.limit).toBeUndefined();
891+
expect(inboxRead.query.orderBy).toBeUndefined();
892+
expect(calls.filter((c) => c.object === 'sys_notification_receipt')).toHaveLength(1);
893+
}
894+
});
895+
896+
it('touches only the addressed user', async () => {
897+
const engine = inboxEngine({
898+
inbox: [
899+
...seedInbox('u1', 350),
900+
{ id: 'x1', user_id: 'u2', notification_id: 'xn1', title: 'X', body_md: 'x', created_at: '2026-01-01T00:00:00.900Z' },
901+
{ id: 'x2', user_id: 'u2', notification_id: 'xn2', title: 'Y', body_md: 'y', created_at: '2026-01-01T00:00:00.901Z' },
902+
],
903+
});
904+
const svc = new MessagingService({ logger, getData: () => engine });
905+
906+
expect((await svc.markAllRead('u1')).readCount).toBe(350);
907+
expect((await svc.listInbox('u2')).unreadCount).toBe(2);
908+
expect(engine.store.sys_notification_receipt.every((r: any) => r.user_id === 'u1')).toBe(true);
909+
});
910+
911+
it('is idempotent — a second sweep writes nothing and reports 0', async () => {
912+
const engine = inboxEngine({ inbox: seedInbox('u1', 350) });
913+
const svc = new MessagingService({ logger, getData: () => engine });
914+
915+
expect((await svc.markAllRead('u1')).readCount).toBe(350);
916+
expect((await svc.markAllRead('u1')).readCount).toBe(0);
917+
expect(engine.store.sys_notification_receipt).toHaveLength(350);
918+
});
919+
920+
it('counts a notification once when several inbox rows materialize it', async () => {
921+
// `readCount` reports NOTIFICATIONS flipped, and the receipt is keyed
922+
// `(notification_id, user_id, channel)` — one row per notification
923+
// however many inbox rows point at it. Feeding the id twice would have
924+
// counted a second upsert that wrote nothing new.
925+
const inbox = seedInbox('u1', 3);
926+
inbox[2].notification_id = 'n1'; // m3 re-materializes n1
927+
const engine = inboxEngine({ inbox });
928+
const svc = new MessagingService({ logger, getData: () => engine });
929+
930+
expect((await svc.markAllRead('u1')).readCount).toBe(2);
931+
expect(engine.store.sys_notification_receipt).toHaveLength(2);
932+
expect((await svc.listInbox('u1')).unreadCount).toBe(0);
933+
});
934+
935+
it('reads the receipt spine best-effort, exactly as listInbox does', async () => {
936+
// Read-state lives on a DIFFERENT object which a minimal stack may not
937+
// have registered (`listInbox` degrades to "everything unread" for the
938+
// same reason). Degrading to "sweep them all" is the safe direction:
939+
// re-marking a read message is idempotent, skipping an unread one is
940+
// the defect this issue is about.
941+
const engine = inboxEngine({
942+
inbox: seedInbox('u1', 3),
943+
receipts: [readReceipt('u1', 1)],
944+
});
945+
const real = engine.find.bind(engine);
946+
engine.find = async (object: string, query: any = {}) => {
947+
if (object === 'sys_notification_receipt') throw new Error('receipts unavailable');
948+
return real(object, query);
949+
};
950+
const svc = new MessagingService({ logger, getData: () => engine });
951+
952+
expect((await svc.markAllRead('u1')).readCount).toBe(3);
953+
});
954+
955+
it('skips rows carrying no event id — they key no receipt at all', async () => {
956+
// Recorded, NOT endorsed. Read-state is keyed by the EVENT id
957+
// (ADR-0030) and the inbox channel writes no receipt for a row without
958+
// one, so there is nothing this sweep can write for it. The old code
959+
// fed `markRead` the inbox ROW id (`listInbox` views it as `nid ??
960+
// String(m.id)`), which inserted a receipt the join never reads back —
961+
// it could not make the row read and still counted itself into
962+
// `readCount`. Skipping it keeps `readCount` honest; #6363's count goes
963+
// on reporting the row as unread, which is the true state. Whether such
964+
// a row should be readable at all is #6448 — a gap in the receipt KEY,
965+
// not in this sweep, and dormant: the single `emit()` ingress always
966+
// carries an event id, so only data written around it can be null.
967+
const inbox = seedInbox('u1', 3);
968+
inbox[0].notification_id = null;
969+
const engine = inboxEngine({ inbox });
970+
const svc = new MessagingService({ logger, getData: () => engine });
971+
972+
expect((await svc.markAllRead('u1')).readCount).toBe(2);
973+
expect(engine.store.sys_notification_receipt).toHaveLength(2);
974+
expect((await svc.listInbox('u1')).unreadCount).toBe(1);
975+
});
976+
977+
it('still degrades to a no-op without a data engine or user id', async () => {
978+
const noData = new MessagingService({ logger });
979+
expect(await noData.markAllRead('u1')).toEqual({ success: true, readCount: 0 });
980+
981+
const svc = new MessagingService({ logger, getData: () => inboxEngine({ inbox: seedInbox('u1', 3) }) });
982+
expect(await svc.markAllRead('')).toEqual({ success: true, readCount: 0 });
983+
});
984+
});

0 commit comments

Comments
 (0)