Skip to content

feat(progress): add idempotent repair-cell persistence - #9

Open
yokoszn wants to merge 5 commits into
docs/pr5450-review-candidatefrom
feat/pr5450-repair-cells
Open

yokoszn wants to merge 5 commits into
docs/pr5450-review-candidatefrom
feat/pr5450-repair-cells

Conversation

@yokoszn

@yokoszn yokoszn commented Aug 23, 2026

Copy link
Copy Markdown

Purpose

Follow-up implementation for the PR open-mercato#5450 review staging branch. This is deliberately a separate PR so the review can distinguish the docs/spec lane from executable changes.

Included

  • Adds progress_job_repair_cells with tenant/org scope and a due-key index.
  • Adds an idempotent primary-key upsert for at-least-once repair delivery.
  • Adds bounded, tenant-scoped due-cell claims with pessimistic partial-write locking.
  • Adds focused unit coverage and a generated Mikro-ORM migration.

Validation

  • yarn build:packages passed.
  • Queue package build passed.
  • Queue package typecheck passed.
  • Focused progress/data-sync tests passed.
  • Local queue strategy: 35/35 passed, including recreated-directory recovery.
  • Full queue package: 104/104 passed in approximately 8.6s with --forceExit.
  • git diff --check passed.
  • Scoped Postgres benchmark and backlog stability passed.

Validation qualification

The queue assertions are green, but the ordinary Jest process still reports an open-handle condition after the suite. --detectOpenHandles --forceExit completes all 104/104 tests; this is recorded as a remaining process-cleanup gate, not presented as a clean Jest exit.

This branch is still not the complete PR open-mercato#5450 solution. Live Postgres concurrent-claim/crash-recovery integration, real Valkey worker-loss testing, workflow latency/RSS benchmarks, and full upstream validation remain open.

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 22be7fdb-f2de-4526-99e4-d96bd82cb355

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be17b9c597

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}, {
orderBy: { dueAt: 'asc', jobId: 'asc' },
limit,
lockMode: LockMode.PESSIMISTIC_PARTIAL_WRITE,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make repair-cell claims transactional

When a reconciler calls this helper with a normal nontransactional EntityManager, MikroORM rejects the pessimistic lock; even if the query is executed in an implicit PostgreSQL transaction, that transaction releases the FOR UPDATE SKIP LOCKED locks before the rows are returned, allowing another replica to select the same cells. Require and verify a caller-owned transaction that performs the claim mutation before commit, or expose a callback that the helper runs inside its own short transaction.

AGENTS.md reference: packages/core/src/modules/progress/AGENTS.md:L91-L93

Useful? React with 👍 / 👎.

cell: input.cell,
dueAt: input.dueAt,
reason: input.reason ?? null,
}, { onConflictAction: 'merge' })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fence stale repair-cell upserts

When two heartbeat or lease updates for the same job are delivered out of order, the unconditional conflict merge lets the older update overwrite the newer dueAt. The reconciler then either hot-loops on a cell whose authoritative lease is still live or removes it and loses the later expiry entirely, leaving a future orphan unrepairable. Include a monotonic lease epoch/version in the conflict predicate, or update dueAt only when the incoming write is still current.

AGENTS.md reference: packages/core/src/modules/progress/AGENTS.md:L91-L93

Useful? React with 👍 / 👎.


export class Migration20260823084507_progress extends Migration {

override up(): void | Promise<void> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Add a down migration for repair cells

When an operator rolls this migration back, there is no down() implementation to remove progress_job_repair_cells, so the schema rollback required by .ai/specs/2026-08-21-background-work-06-leased-jobs-in-progress.md:320 cannot restore the pre-change database state. Add a down migration that drops the index and table so rollback and subsequent forward migration remain reliable.

Useful? React with 👍 / 👎.

@yokoszn

yokoszn commented Aug 23, 2026

Copy link
Copy Markdown
Author

Implementation update

The branch now includes durable repair-cell lease semantics:

  • bounded due-cell selection remains tenant/org scoped;
  • claims acquire a persisted lease token and expiry inside a transaction;
  • attempts increment on claim;
  • acknowledgement and release require the lease token;
  • expired leases become claimable again after a worker crash.

Validation on the branch: focused progress tests pass (3 tests), package build passed, and the core package typecheck passed after generation.

This still does not claim the full open-mercato#5450 implementation is complete. The next gates are integration with actual data-sync/workflow transitions and live Postgres/queue latency, crash, backlog-drain, and tenant-isolation tests.

@yokoszn

yokoszn commented Aug 23, 2026

Copy link
Copy Markdown
Author

Verification update

Implemented

  • data-sync import/export workers now clear repair cells after successful delivery and record a scoped due repair cell on failure.
  • local queue filesystem watcher now retries promptly after watcher errors instead of waiting for the idle fallback interval.

Passing

  • data-sync worker/lib tests: 15 passed.
  • progress and repair-cell tests: 3 passed.
  • package build: 26/26 packages passed.
  • scoped Postgres benchmark and backlog stability: passed.

Postgres measured result

Using equivalent tenant/org predicates and fixed batch size:

Live rows Baseline p50 Candidate p50 Candidate plan
1 0.083 ms 0.094 ms tiny-table sequential scan
10 0.072 ms 0.087 ms tiny-table sequential scan
100 0.111 ms 0.114 ms tiny-table sequential scan
1,000 0.325 ms 0.112 ms index scan
10,000 1.711 ms 0.383 ms index scan
100,000 13.551 ms 0.433 ms index scan
1,000,000 54.135 ms 0.375 ms index scan

The small-table sequential plan is intentional PostgreSQL behavior; forcing an index there is slower. At material scale the candidate rows read remain bounded by the due batch rather than live-row count.

Remaining red gate

The queue package suite is 101/104 passing. One directory-recreation wake-up test still times out after watcher recovery; the two ordinary event-wakeup tests pass. This is not being marked green until that test is fixed and the full queue suite is rerun.

The benchmark artifact and scoped-query correction are in the private evidence repo commit 91bfed1.

@yokoszn

yokoszn commented Aug 23, 2026

Copy link
Copy Markdown
Author

Verification correction

The earlier 101/104 queue status is superseded. After removing stale Jest/Yarn processes and clearing approximately 18 GB of Cargo incremental artifacts:

  • local strategy: 35/35 passed, including directory recreation recovery;
  • full queue package: 104/104 passed in approximately 8.6s with --forceExit;
  • queue package typecheck and build passed;
  • git diff --check passed.

The ordinary Jest process still reports an open-handle condition after all assertions complete. That remains an explicit cleanup gate; the result is not described as a clean Jest exit. The remaining PR #9 gates are real Postgres concurrent-claim/crash-recovery integration, Valkey worker-loss testing, workflow latency/RSS measurements, and the full upstream validation sequence.

@yokoszn

yokoszn commented Aug 23, 2026

Copy link
Copy Markdown
Author

Codex findings addressed

Pushed in commit fd56d4ba1.

  • Transactional claims: added withClaimedRepairCells(...), which keeps row selection, lease mutation, and caller work inside one caller-owned MikroORM transaction. The claim helper uses a transaction-internal implementation to avoid nested transactions.
  • Stale upserts: added monotonic leaseEpoch fencing and a PostgreSQL ON CONFLICT ... WHERE excluded.lease_epoch >= current.lease_epoch predicate. Older out-of-order deliveries cannot overwrite a newer repair coordinate.
  • Rollback: added down() to the original repair-cell migration and added a reversible follow-up migration for lease_epoch.

Validation: core typecheck passed, focused repair-cell tests 3/3 passed, core build passed, and git diff --check passed. The local queue recovery implementation and 104/104 queue result remain in the PR description.

The direct claimDueRepairCells API remains a self-contained transactional claim for compatibility; reconciler code that must perform follow-up work under the claim lock should use withClaimedRepairCells(...).

@yokoszn

yokoszn commented Aug 24, 2026

Copy link
Copy Markdown
Author

The implementation lane has now been integrated with PR 8 onto the PR open-mercato#5450 base in integration PR 10.

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.

1 participant