[Refactor] Убрать дублирование reservation и claim-протокола sync #119

New Issue

2026-10-20T05:00:41+03:00

codex-bot commented

2026-10-20 05:00:41 +03:00

Проблема

Аудит main/audits/2026-10-19-code-quality-architecture/02-findings.md, CQ-004/CQ-005 и 03-overengineering.md, OE-01: протокол запуска/reservation/drain повторён в шести sync-сервисах; claim к одной таблице tbankSyncWork реализован тремя способами. Изменение инварианта приходится согласовывать в нескольких местах. Фактическое расхождение поведения трёх claim-реализаций аудит не подтвердил — сначала проверить их семантику и транзакционные границы.

Результат и scope

Устранить доказанное дублирование протокола у существующего владельца, мигрируя потребителей по одному с проверкой поведения. Не навязывать всем сервисам одну семантику: Map и Set сейчас по-разному обрабатывают повторный вызов; внешние транзакции и lifecycle каждого run должны сохраняться. ReservationRunner из отчёта — гипотеза реализации, не обязательный результат.

Acceptance Criteria

Для шести runners и трёх claim-путей перечислены сохраняемые инварианты (join/dedup, reserve/finish, failure/drain, restart и границы транзакций); подтверждено, что объединяется только общий протокол.
Claim/reservation больше не поддерживается несколькими независимыми реализациями одного инварианта; ответственное место и оставшиеся намеренные различия явно видны.
Переход каждого потребителя проверен релевантными concurrency/recovery тестами; проверяется сохранение результата при параллельных запросах, сбое и рестарте, без ложной зелёности теста.
Сборка, архитектурные проверки и релевантные тесты backend проходят.

Связано с main#22 (часовая синхронизация), backend#104 (барьеры в тестах операций) и backend#118 (внутренняя граница tbank-sync); их разные AC не заменяют эту задачу. Источник: аудит 2026-10-19, план, пункт 8.

## Проблема Аудит `main/audits/2026-10-19-code-quality-architecture/02-findings.md`, CQ-004/CQ-005 и `03-overengineering.md`, OE-01: протокол запуска/reservation/drain повторён в шести sync-сервисах; claim к одной таблице `tbankSyncWork` реализован тремя способами. Изменение инварианта приходится согласовывать в нескольких местах. Фактическое расхождение поведения трёх claim-реализаций аудит не подтвердил — сначала проверить их семантику и транзакционные границы. ## Результат и scope Устранить доказанное дублирование протокола у существующего владельца, мигрируя потребителей по одному с проверкой поведения. Не навязывать всем сервисам одну семантику: `Map` и `Set` сейчас по-разному обрабатывают повторный вызов; внешние транзакции и lifecycle каждого run должны сохраняться. `ReservationRunner` из отчёта — гипотеза реализации, не обязательный результат. ## Acceptance Criteria - [ ] Для шести runners и трёх claim-путей перечислены сохраняемые инварианты (join/dedup, reserve/finish, failure/drain, restart и границы транзакций); подтверждено, что объединяется только общий протокол. - [ ] Claim/reservation больше не поддерживается несколькими независимыми реализациями одного инварианта; ответственное место и оставшиеся намеренные различия явно видны. - [ ] Переход каждого потребителя проверен релевантными concurrency/recovery тестами; проверяется сохранение результата при параллельных запросах, сбое и рестарте, без ложной зелёности теста. - [ ] Сборка, архитектурные проверки и релевантные тесты backend проходят. Связано с [main#22](https://git.personal.ksv741.ru/WealthLedger/main/issues/22) (часовая синхронизация), [backend#104](https://git.personal.ksv741.ru/WealthLedger/backend/issues/104) (барьеры в тестах операций) и [backend#118](https://git.personal.ksv741.ru/WealthLedger/backend/issues/118) (внутренняя граница `tbank-sync`); их разные AC не заменяют эту задачу. Источник: аудит 2026-10-19, план, пункт 8.

codex-bot added the

Priority

Medium

3

Kind/Refactor labels 2026-10-20 05:00:41 +03:00

ksv741 referenced this issue from a commit

2026-11-03 22:32:42 +03:00

refactor: share sync reservation join rule (WealthLedger/backend#119)

codex-bot referenced a pull request that will close this issue

2026-11-03 22:32:50 +03:00

refactor: share sync reservation join rule (WealthLedger/backend#119) #162

codex-bot commented

2026-11-03 22:43:06 +03:00

PR backend#162 (refactor/119-sync-reservation-skeleton, commit 1bc254a) is ready for review; CI verify/commits/security are green.

What changed

The duplicated join predicate of sync reservations is now one function, isJoinableRunningAttempt (type guard next to TbankSyncAttempt, re-exported from tbank.api.ts).
Used by: tbank-access.service.ts (membership), instrument-metadata.repository.ts, catalog-discovery.repository.ts.
Works with Set and Map (only .has is used), so portfolio's Map can adopt it later without an adapter.

Scope decision (differs from the Issue's suggestion)

The Issue suggested a shared reserve skeleton; only the join predicate is shared. The reserve blocks differ in scope key, intent, priority, membership TTL pre-check, findUniqueOrThrow, and the active structure. A generic runner would take 6–7 parameters and be bigger than the duplicates. This follows the Issue's own note that ReservationRunner is a hypothesis, not a requirement.

Not changed (different rules, out of path A): candle join (raises priority, rewrites intent), screen join (checks progress.dispatched and children), portfolio (left as-is for now: it uses pending Map and the same rule could be adopted in a follow-up).

Characterization

New test/tbank-membership-reservation.e2e-spec.ts (2 tests, real SQLite): same-process join returns the same id/attempt; relaunch after the attempt leaves the active set; a foreign process never joins.
Mutation check: removing the active-set check makes the test fail.

Verification

Before changes (develop): unit 310/310, e2e 322/322, openapi OK.
After: tsc app+test 0 errors; unit 310/310; e2e 324/324; openapi OK; lint OK.

Not verified

No production runtime check; SQLite harness only.
Full npm run verify was not run as one command; its steps were run separately.
Sync-state logic in TbankSyncService.status and the hourly scheduler is unchanged.

Not closed yet: waiting for merge approval. Closing via Closes in the PR when merged.

PR backend#162 (`refactor/119-sync-reservation-skeleton`, commit `1bc254a`) is ready for review; CI verify/commits/security are green. ## What changed - The duplicated join predicate of sync reservations is now one function, `isJoinableRunningAttempt` (type guard next to `TbankSyncAttempt`, re-exported from `tbank.api.ts`). - Used by: `tbank-access.service.ts` (membership), `instrument-metadata.repository.ts`, `catalog-discovery.repository.ts`. - Works with `Set` and `Map` (only `.has` is used), so portfolio's `Map` can adopt it later without an adapter. ## Scope decision (differs from the Issue's suggestion) The Issue suggested a shared reserve skeleton; only the join predicate is shared. The reserve blocks differ in scope key, intent, priority, membership TTL pre-check, `findUniqueOrThrow`, and the active structure. A generic runner would take 6–7 parameters and be bigger than the duplicates. This follows the Issue's own note that `ReservationRunner` is a hypothesis, not a requirement. - Not changed (different rules, out of path A): candle join (raises priority, rewrites intent), screen join (checks `progress.dispatched` and children), portfolio (left as-is for now: it uses `pending` Map and the same rule could be adopted in a follow-up). ## Characterization - New `test/tbank-membership-reservation.e2e-spec.ts` (2 tests, real SQLite): same-process join returns the same `id`/attempt; relaunch after the attempt leaves the active set; a foreign process never joins. - Mutation check: removing the active-set check makes the test fail. ## Verification - Before changes (develop): unit 310/310, e2e 322/322, openapi OK. - After: `tsc` app+test 0 errors; unit 310/310; e2e 324/324; openapi OK; lint OK. ## Not verified - No production runtime check; SQLite harness only. - Full `npm run verify` was not run as one command; its steps were run separately. - Sync-state logic in `TbankSyncService.status` and the hourly scheduler is unchanged. Not closed yet: waiting for merge approval. Closing via `Closes` in the PR when merged.

codex-bot closed this issue

2026-11-03 22:53:55 +03:00

codex-bot referenced this issue from a commit

2026-11-03 22:53:56 +03:00

refactor: share sync reservation join rule (WealthLedger/backend#119) (#162)

codex-bot commented

2026-11-03 22:54:10 +03:00

Merged as fa0a6e5 (squash of PR backend#162). Closed by Closes.

Acceptance criteria

Invariants for the reserve paths are listed and characterized. Membership join/relaunch/foreign-process is covered by the new test/tbank-membership-reservation.e2e-spec.ts; metadata coalescing, abandoned-work recovery and superseded-attempt fencing are covered by the existing tbank-metadata-persistence e2e; catalog and hourly lifecycle by their existing e2e.
The join rule is in one place (isJoinableRunningAttempt, tbank-access.service.ts) and used by membership, metadata and catalog-discovery. Candle, screen and portfolio keep their own rules on purpose (documented in the PR).
Concurrency and restart behaviour verified by the characterization tests and the existing e2e; a mutation that removes the active-set check fails the new test.
Build, types and relevant backend tests pass (see below).

Scope note (differs from the Issue body)

The full reserve skeleton (scope, intent, priority, TTL pre-check, findUniqueOrThrow, Set vs Map) was not extracted. Only the join predicate was shared, because the rest differs by design and a generic runner would add parameters without removing logic. The Issue itself called ReservationRunner a hypothesis, not a requirement.

Verification (on the PR branch; develop identical after squash)

Before changes: unit 310/310, e2e 322/322, openapi:check OK.
After: tsc app and test configs 0 errors; unit 310/310; e2e 324/324 (322 + 2 new); openapi:check OK; npm run lint OK.
CI on PR backend#162: commits, security, verify — success.

Not verified

Production runtime; only the SQLite e2e harness.
npm run verify was not run as one command locally; its steps were run separately (CI verify on the PR was green).
Portfolio reserve (Map-based) still has its own copy of the join rule; can adopt isJoinableRunningAttempt in a follow-up.
TbankSyncService.status and the hourly scheduler behaviour are unchanged.

API contract

No API or OpenAPI change.

Merged as `fa0a6e5` (squash of PR backend#162). Closed by `Closes`. ## Acceptance criteria - [x] Invariants for the reserve paths are listed and characterized. Membership join/relaunch/foreign-process is covered by the new `test/tbank-membership-reservation.e2e-spec.ts`; metadata coalescing, abandoned-work recovery and superseded-attempt fencing are covered by the existing `tbank-metadata-persistence` e2e; catalog and hourly lifecycle by their existing e2e. - [x] The join rule is in one place (`isJoinableRunningAttempt`, `tbank-access.service.ts`) and used by membership, metadata and catalog-discovery. Candle, screen and portfolio keep their own rules on purpose (documented in the PR). - [x] Concurrency and restart behaviour verified by the characterization tests and the existing e2e; a mutation that removes the active-set check fails the new test. - [x] Build, types and relevant backend tests pass (see below). ## Scope note (differs from the Issue body) The full reserve skeleton (scope, intent, priority, TTL pre-check, `findUniqueOrThrow`, Set vs Map) was not extracted. Only the join predicate was shared, because the rest differs by design and a generic runner would add parameters without removing logic. The Issue itself called `ReservationRunner` a hypothesis, not a requirement. ## Verification (on the PR branch; develop identical after squash) - Before changes: unit 310/310, e2e 322/322, `openapi:check` OK. - After: `tsc` app and test configs 0 errors; unit 310/310; e2e 324/324 (322 + 2 new); `openapi:check` OK; `npm run lint` OK. - CI on PR backend#162: commits, security, verify — success. ## Not verified - Production runtime; only the SQLite e2e harness. - `npm run verify` was not run as one command locally; its steps were run separately (CI `verify` on the PR was green). - Portfolio reserve (`Map`-based) still has its own copy of the join rule; can adopt `isJoinableRunningAttempt` in a follow-up. - `TbankSyncService.status` and the hourly scheduler behaviour are unchanged. ## API contract No API or OpenAPI change.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: WealthLedger/backend#119