-
Notifications
You must be signed in to change notification settings - Fork 0
feat: integrate PR 5450 review specs and implementation #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: review/pr5450-base
Are you sure you want to change the base?
Changes from all commits
56d9b72
2c5f0a0
4618461
8585f8f
e411451
1de5b09
d8e6770
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # Local Queue Filesystem Watch Recovery | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed implementation specification for the local queue strategy. This | ||
| specification makes the existing behavior explicit before changing it. | ||
|
|
||
| ## Contract | ||
|
|
||
| Filesystem notifications are an acceleration path, never the delivery | ||
| guarantee. A continuous worker MUST process a committed queue entry when: | ||
|
|
||
| 1. the queue file is atomically replaced; | ||
| 2. the queue directory is moved away and recreated; | ||
| 3. `fs.watch` cannot be installed or later emits an error; or | ||
| 4. the host exhausts its inotify watch quota (`ENOSPC`). | ||
|
|
||
| The worker MUST preserve at-least-once processing. A missed notification may | ||
| increase latency, but MUST NOT lose a committed entry. Idle workers MUST NOT | ||
| read the queue file more frequently than the configured idle safety interval. | ||
|
|
||
| ## Failure model | ||
|
|
||
| On Linux, `fs.watch(file)` is backed by an inotify watch attached to the inode | ||
| identified when the watch is installed. The local queue writer replaces | ||
| `queue.json` during an atomic write. The replacement has a new inode, so a | ||
| watch on the old file is not a durable subscription to future queue files. | ||
| Moving the containing directory is stronger: the watched pathname disappears | ||
| and a later directory with the same pathname is a different filesystem object. | ||
|
|
||
| `ENOSPC` from `fs.watch` means the process has exhausted the kernel's inotify | ||
| watch quota; it does not mean the disk is full. Retrying continuously increases | ||
| log volume and CPU usage without changing the quota. Retries therefore MUST be | ||
| bounded and the normal safety poll MUST remain authoritative. | ||
|
|
||
| ## Required design | ||
|
|
||
| The implementation MUST combine: | ||
|
|
||
| - a file watcher for low-latency wakeups while the current queue inode exists; | ||
| - identity comparison (`device`, `inode`) before reusing a watcher; | ||
| - re-arm after a file `rename` event; | ||
| - a parent-directory recovery signal for queue-directory replacement, when | ||
| available; | ||
| - bounded watcher re-arm attempts after setup/error failure; and | ||
| - the existing idle safety poll as the final correctness mechanism. | ||
|
|
||
| The implementation MUST NOT add an unconditional high-frequency queue-file | ||
| read loop. Recovery probes may be enabled only while the watched directory is | ||
| absent or the watcher is unavailable, and MUST stop once the watcher is | ||
| re-established or the bounded recovery window expires. | ||
|
|
||
| ## Verification matrix | ||
|
|
||
| | Case | Expected result | | ||
| |---|---| | ||
| | New enqueue with watcher | handler runs before safety interval | | ||
| | Atomic queue-file replacement | handler runs and watcher is re-armed | | ||
| | Queue directory moved/recreated | first entry is recovered by polling; second entry is delivered without waiting for the long idle interval | | ||
| | Watch setup throws | no job loss; bounded fallback polling remains active | | ||
| | Watch emits `ENOSPC` | no retry storm; no job loss; bounded retries then safety polling | | ||
| | Idle worker | no queue-file reads before the configured idle interval | | ||
| | Worker restart | previous watcher is closed exactly once | | ||
|
|
||
| ## Acceptance criteria | ||
|
|
||
| The local queue strategy test file MUST pass in full. The full queue package | ||
| suite MUST pass without an open-handle timeout or unbounded watcher retry log. | ||
| The implementation change MUST report the observed event latency and fallback | ||
| latency separately; an event-driven result does not prove recovery behavior. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { clearDataSyncRepairCell, recordDataSyncRepairCell } from '../repair-cell' | ||
|
|
||
| describe('data-sync repair-cell integration', () => { | ||
| it('records a scoped repair cell for a failed delivery', async () => { | ||
| const em = { upsert: jest.fn().mockResolvedValue(undefined) } | ||
| await recordDataSyncRepairCell(em as never, 'progress-1', { tenantId: 'tenant-1', organizationId: 'org-1' }, 'import', 'upstream timeout') | ||
| expect(em.upsert).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ | ||
| jobId: 'progress-1', tenantId: 'tenant-1', organizationId: 'org-1', cell: 'data_sync.import', reason: 'upstream timeout', | ||
| }), expect.anything()) | ||
| }) | ||
|
|
||
| it('clears the same scoped repair cell after successful delivery', async () => { | ||
| const em = { nativeDelete: jest.fn().mockResolvedValue(1) } | ||
| await clearDataSyncRepairCell(em as never, 'progress-1', { tenantId: 'tenant-1', organizationId: 'org-1' }) | ||
| expect(em.nativeDelete).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ | ||
| jobId: 'progress-1', tenantId: 'tenant-1', organizationId: 'org-1', | ||
| })) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import type { EntityManager } from '@mikro-orm/postgresql' | ||
| import { removeRepairCell, upsertRepairCell } from '../../progress/lib/repair-cells' | ||
|
|
||
| type RepairScope = { | ||
| tenantId: string | ||
| organizationId?: string | null | ||
| } | ||
|
|
||
| export async function clearDataSyncRepairCell( | ||
| em: EntityManager, | ||
| progressJobId: string | null | undefined, | ||
| scope: RepairScope, | ||
| ): Promise<void> { | ||
| if (!progressJobId) return | ||
| await removeRepairCell(em, progressJobId, scope) | ||
| } | ||
|
|
||
| export async function recordDataSyncRepairCell( | ||
| em: EntityManager, | ||
| progressJobId: string | null | undefined, | ||
| scope: RepairScope, | ||
| direction: 'import' | 'export', | ||
| reason: string, | ||
| ): Promise<void> { | ||
| if (!progressJobId) return | ||
| await upsertRepairCell(em, { | ||
| jobId: progressJobId, | ||
| tenantId: scope.tenantId, | ||
| organizationId: scope.organizationId, | ||
| cell: `data_sync.${direction}`, | ||
| dueAt: new Date(), | ||
| reason, | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| } from '../lib/queue-policy' | ||
| import { failAbandonedRun } from '../lib/abandoned-run' | ||
| import { createLogger } from '@open-mercato/shared/lib/logger' | ||
| import { clearDataSyncRepairCell, recordDataSyncRepairCell } from '../lib/repair-cell' | ||
|
|
||
| const logger = createLogger('data_sync').child({ component: 'sync-import' }) | ||
|
|
||
|
|
@@ -41,14 +42,20 @@ export default async function handle(job: QueuedJob<SyncJobPayload>, ctx: Handle | |
| try { | ||
| const engine = ctx.resolve<SyncEngine>('dataSyncEngine') | ||
| await engine.runImport(job.payload.runId, job.payload.batchSize, job.payload.scope) | ||
| const em = ctx.resolve<import('@mikro-orm/postgresql').EntityManager>('em') | ||
| const syncRunService = ctx.resolve<SyncRunService>('dataSyncRunService') | ||
| const run = await syncRunService.getRun(job.payload.runId, job.payload.scope) | ||
| await clearDataSyncRepairCell(em, run?.progressJobId, job.payload.scope) | ||
|
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a queue retry after this handler has marked the run Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : 'Data sync import worker failed' | ||
| const errorStack = error instanceof Error ? error.stack : undefined | ||
|
|
||
| try { | ||
| const syncRunService = ctx.resolve<SyncRunService>('dataSyncRunService') | ||
| const em = ctx.resolve<import('@mikro-orm/postgresql').EntityManager>('em') | ||
| const progressService = ctx.resolve<ProgressService>('progressService') | ||
| const run = await syncRunService.getRun(job.payload.runId, job.payload.scope) | ||
| await recordDataSyncRepairCell(em, run?.progressJobId, job.payload.scope, 'import', message) | ||
|
Comment on lines
+55
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the new repair table is unavailable—for example during schema drift or a rolling deployment—or its conditional upsert otherwise fails, this awaited auxiliary write aborts the entire nested finalization block before AGENTS.md reference: packages/core/src/modules/progress/AGENTS.md:L7-L10 Useful? React with 👍 / 👎. |
||
|
|
||
| if (run && run.status !== 'completed' && run.status !== 'failed' && run.status !== 'cancelled') { | ||
| await syncRunService.markStatus(run.id, 'failed', job.payload.scope, message) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test constructs an entity manager with only
upsert, butrecordDataSyncRepairCell()delegates toupsertRepairCell(), which callsem.getConnection().execute(...). Consequently, running this new test throwsTypeError: em.getConnection is not a functionbefore reaching the assertion; mock the connection API and assert the executed statement instead.Useful? React with 👍 / 👎.