From 9bd83fa52d7c19a7501e973726f05ee2f008e14d Mon Sep 17 00:00:00 2001 From: Arvin Ansari Date: Mon, 31 Aug 2026 15:00:00 +0200 Subject: [PATCH] refactor(core): Narrow the activity event repository to its write path (no-changelog) Review follow-ups on the activity event table. - `record` no longer accepts an `EntityManager`. Handing a driver handle to callers is the leak `BaseRepository` and `TransactionRunner` exist to prevent, and no caller wanted it: an entry describes something that already happened, so it must not roll back with the operation that produced it. - Drop `findById` and `findProjectIdForResource`. Both are reader work, both arrived with no caller and no test. - `projectIds` is required on a feed query. It is the access boundary, and an omitted scope read every project on the instance. A comment saying "required" is not an access check. - A zero retention cap deletes everything instead of asking the driver for a negative offset. Two comments described mechanisms that do not hold. Pruning deletes the oldest rows, so it never frees the highest SQLite rowid; only emptying the table does. And a project deletion cannot be recorded against the project, because the resource vocabulary has no entry for one. --- .../postgres-schema/public.activity_event.md | 2 +- .../@n8n/db/src/entities/activity-event.ts | 10 ++- .../1787741452169-CreateActivityEventTable.ts | 10 +-- .../activity-event.repository.test.ts | 19 +++++- .../repositories/activity-event.repository.ts | 62 ++++++------------- 5 files changed, 49 insertions(+), 54 deletions(-) diff --git a/docs/generated/postgres-schema/public.activity_event.md b/docs/generated/postgres-schema/public.activity_event.md index 7e347256b26..49f1613de15 100644 --- a/docs/generated/postgres-schema/public.activity_event.md +++ b/docs/generated/postgres-schema/public.activity_event.md @@ -9,7 +9,7 @@ | createdAt | timestamp(3) with time zone | CURRENT_TIMESTAMP(3) | false | | | | | data | json | | true | | | Minimal detail that makes an entry meaningful unexpanded (a run status and failing node, a save node delta). Size-capped on write; no user ids | | id | integer | | false | | | | -| projectId | varchar(36) | | true | | [public.project](public.project.md) | Scopes the entry to one project. NULL for instance-level entries — including a project deletion, which would otherwise cascade away the record of itself | +| projectId | varchar(36) | | true | | [public.project](public.project.md) | Scopes the entry to one project. NULL for entries about the instance rather than one project | | resourceId | varchar(36) | | true | | | Id of the resource, for fetching the full record. No FK: entries outlive it | | resourceName | text | | true | | | Name at the time of the entry, denormalised so a row reads without a join and survives the resource being deleted. Truncated on write | | resourceType | varchar(32) | | true | | | What `resourceId` points at; see ActivityResourceType in @n8n/db. NULL when an entry is about the instance rather than a resource | diff --git a/packages/@n8n/db/src/entities/activity-event.ts b/packages/@n8n/db/src/entities/activity-event.ts index ad13ffd5cd4..d142115444d 100644 --- a/packages/@n8n/db/src/entities/activity-event.ts +++ b/packages/@n8n/db/src/entities/activity-event.ts @@ -35,8 +35,9 @@ export class ActivityEvent extends WithCreatedAt { /** * Autoincrement int, not the usual nanoid: the feed orders by id, pages on it as a cursor, and * records the high-water mark of what a thread was already shown. On SQLite this is a rowid - * alias, so ids are reused once the top rows are deleted, which pruning does — never compare a - * stored cursor against a feed that has been pruned beneath it. + * alias: pruning deletes the oldest rows and so never frees the highest id, but emptying the + * table does, and ids then restart. A stored cursor is only meaningful against a feed that has + * held at least one row throughout. */ @PrimaryGeneratedColumn() id: number; @@ -62,6 +63,11 @@ export class ActivityEvent extends WithCreatedAt { @Column({ type: 'uuid', nullable: true }) userId: string | null; + /** + * Null for entries about the instance rather than one project. A project deletion cascades its + * entries away; recording that deletion needs a resource vocabulary that covers projects, which + * is why no writer emits one yet. + */ @Column({ type: 'varchar', length: 36, nullable: true }) projectId: string | null; diff --git a/packages/@n8n/db/src/migrations/common/1787741452169-CreateActivityEventTable.ts b/packages/@n8n/db/src/migrations/common/1787741452169-CreateActivityEventTable.ts index a30a4c7d85f..4cf07a9667a 100644 --- a/packages/@n8n/db/src/migrations/common/1787741452169-CreateActivityEventTable.ts +++ b/packages/@n8n/db/src/migrations/common/1787741452169-CreateActivityEventTable.ts @@ -38,8 +38,8 @@ export class CreateActivityEventTable1787741452169 implements ReversibleMigratio column('projectId') .varchar(36) .comment( - 'Scopes the entry to one project. NULL for instance-level entries — including ' + - 'a project deletion, which would otherwise cascade away the record of itself', + 'Scopes the entry to one project. NULL for entries about the instance rather ' + + 'than one project', ), column('resourceType') .varchar(32) @@ -69,9 +69,9 @@ export class CreateActivityEventTable1787741452169 implements ReversibleMigratio columnName: 'id', onDelete: 'SET NULL', }) - // CASCADE: a deleted project's history is unreachable by every read, which are all - // project-scoped. The one entry that must survive a deletion is written at instance - // level instead, with the project id in `resourceId`. + // CASCADE: a deleted project's history is unreachable by every read, since all of them + // are project-scoped. Recording the deletion itself would need an entry outside the + // project, which no writer emits today. .withForeignKey('projectId', { tableName: PROJECT_TABLE, columnName: 'id', diff --git a/packages/@n8n/db/src/repositories/__tests__/activity-event.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/activity-event.repository.test.ts index 8d6b6400259..fd9dfc6fe0f 100644 --- a/packages/@n8n/db/src/repositories/__tests__/activity-event.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/activity-event.repository.test.ts @@ -98,10 +98,15 @@ describe('ActivityEventRepository', () => { it('combines both id bounds instead of letting one overwrite the other', async () => { entityManager.find.mockResolvedValueOnce([]); - await repository.findFeed({ limit: 10, afterId: 5, beforeId: 40 }); + await repository.findFeed({ + projectIds: ['project1'], + limit: 10, + afterId: 5, + beforeId: 40, + }); expect(entityManager.find).toHaveBeenCalledWith(ActivityEvent, { - where: { id: And(MoreThan(5), LessThan(40)) }, + where: { projectId: In(['project1']), id: And(MoreThan(5), LessThan(40)) }, order: { id: 'DESC' }, take: 10, }); @@ -147,6 +152,16 @@ describe('ActivityEventRepository', () => { expect(deleted).toBe(7); }); + it('deletes everything when the cap is zero, rather than asking for a negative offset', async () => { + entityManager.delete.mockResolvedValueOnce({ affected: 3, raw: [] }); + + const deleted = await repository.deleteBeyondNewest(0); + + expect(entityManager.find).not.toHaveBeenCalled(); + expect(entityManager.delete).toHaveBeenCalledWith(ActivityEvent, {}); + expect(deleted).toBe(3); + }); + it('does nothing when the table holds fewer entries than the cap', async () => { entityManager.find.mockResolvedValueOnce([]); diff --git a/packages/@n8n/db/src/repositories/activity-event.repository.ts b/packages/@n8n/db/src/repositories/activity-event.repository.ts index 8be406315a7..59793b0b3f6 100644 --- a/packages/@n8n/db/src/repositories/activity-event.repository.ts +++ b/packages/@n8n/db/src/repositories/activity-event.repository.ts @@ -1,6 +1,6 @@ import { Service } from '@n8n/di'; -import { And, DataSource, In, LessThan, MoreThan, Not, IsNull, Repository } from '@n8n/typeorm'; -import type { EntityManager, FindOptionsWhere } from '@n8n/typeorm'; +import { And, DataSource, In, LessThan, MoreThan, Repository } from '@n8n/typeorm'; +import type { FindOptionsWhere } from '@n8n/typeorm'; import type { IDataObject } from 'n8n-workflow'; import { ActivityEvent } from '../entities'; @@ -20,11 +20,11 @@ export type ActivityEventInput = Pick & export type ActivityFeedQuery = { limit: number; /** - * The projects whose entries the caller may see. Required for any agent-facing read: without - * it a read spans every project on the instance, including ones the user cannot open. - * Entries with no project are excluded, since there is no way to prove they are in scope. + * The projects whose entries the caller may see. Required, not optional: an omitted scope + * would read every project on the instance, including ones the user cannot open. Entries + * with no project are excluded, since there is no way to prove they are in scope. */ - projectIds?: string[]; + projectIds: string[]; userId?: string; resourceId?: string; category?: ActivityEvent['category']; @@ -42,12 +42,11 @@ export class ActivityEventRepository extends Repository { /** * The only write path, so the row-size guarantees hold whatever the caller passes. - * `trx` lets a caller bind the entry to its own unit of work; the event relay does not, - * because an entry describes something that already happened. + * Deliberately not transaction-aware: an entry describes something that already happened, so + * it must not be rolled back with the operation that produced it. */ - async record(input: ActivityEventInput, trx?: EntityManager): Promise { - const repository = trx ? trx.getRepository(ActivityEvent) : this; - await repository.insert({ + async record(input: ActivityEventInput): Promise { + await this.insert({ category: input.category, action: input.action, userId: input.userId ?? null, @@ -66,13 +65,11 @@ export class ActivityEventRepository extends Repository { * the current user's own actions apart from everyone else's. */ async findFeed(query: ActivityFeedQuery): Promise { - const where: FindOptionsWhere = {}; - if (query.projectIds !== undefined) { - // An empty allowance means nothing is visible, not everything — `In([])` would match no - // row on Postgres but is worth being explicit about rather than relying on it. - if (query.projectIds.length === 0) return []; - where.projectId = In(query.projectIds); - } + // An empty allowance means nothing is visible, not everything — `In([])` would match no + // row on Postgres but is worth being explicit about rather than relying on it. + if (query.projectIds.length === 0) return []; + + const where: FindOptionsWhere = { projectId: In(query.projectIds) }; if (query.userId !== undefined) where.userId = query.userId; if (query.resourceId !== undefined) where.resourceId = query.resourceId; if (query.category !== undefined) where.category = query.category; @@ -105,32 +102,6 @@ export class ActivityEventRepository extends Repository { }); } - /** One entry by id, for expanding it. Callers check its project against their own scope. */ - async findById(id: number): Promise { - return await this.findOne({ where: { id } }); - } - - /** - * The project a resource was last seen in, from the log's own earlier entries. - * - * This is the only way to scope an entry recorded after the resource is gone: by the time - * `workflow-deleted` fires, the sharing rows that would name its project are deleted too. - * Without it such entries have no project, and an agent-facing read has to drop them — which - * would silently lose deletions, the entries most worth surfacing. - */ - async findProjectIdForResource( - resourceType: NonNullable, - resourceId: string, - ): Promise { - const [latest] = await this.find({ - where: { resourceType, resourceId, projectId: Not(IsNull()) }, - order: { id: 'DESC' }, - take: 1, - select: { projectId: true }, - }); - return latest?.projectId ?? undefined; - } - /** Retention by age. Returns how many entries went, so a caller can log a sweep worth noticing. */ async deleteOlderThan(cutoff: Date): Promise { const { affected } = await this.delete({ createdAt: LessThan(cutoff) }); @@ -143,6 +114,9 @@ export class ActivityEventRepository extends Repository { * deletes below it, rather than counting rows twice. */ async deleteBeyondNewest(keep: number): Promise { + // `skip` is an offset, so a zero cap would ask for -1. Keeping nothing means deleting all. + if (keep <= 0) return (await this.delete({})).affected ?? 0; + const [oldestKept] = await this.find({ order: { id: 'DESC' }, skip: keep - 1,