Skip to content

feat: Coalesce link extraction - #11611

Open
arvid-e wants to merge 26 commits into
feat/185872-backlinksfrom
feat/187679-coalesce-link-extraction
Open

feat: Coalesce link extraction#11611
arvid-e wants to merge 26 commits into
feat/185872-backlinksfrom
feat/187679-coalesce-link-extraction

Conversation

@arvid-e

@arvid-e arvid-e commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

https://redmine.weseek.co.jp/issues/185820
https://redmine.weseek.co.jp/issues/187679

Summary

Replaces the inline per-event link extraction with a coalescing, paced queue (task B2.2), plus the review fixes that came out of it.

Coalescing upsert queue (page-link-upsert-queue.ts)

  • The create/update handlers now only mark a page id dirty; a paced timer drains a bounded number of ids per tick (BACKLINKS_DRAIN_INTERVAL_MS / BACKLINKS_MAX_PAGES_PER_DRAIN, default 1000 ms × 3 pages) and awaits one extraction at a time, so an editing burst no longer becomes a blocking spree of markdown parses.
  • The queue holds ids, not the emitted documents, so repeated saves of one page collapse into a single extraction over the latest stored body. The drain re-reads the page (handlePageUpsertById).
  • Ids leave the set before processing, so a save landing mid-drain re-enqueues and gets a fresh run instead of being swallowed. The timer is unref()ed — dropped work self-heals on the next edit.
  • Coalescing opened a delete/upsert race that could not exist before: a soft delete keeps the _id, so the drain would index a source now under /trash. The drain now declines a page with STATUS_DELETED (keyed on deleted rather than published, because a legacy page's null status means published).

Perf

  • The unified / remark / rehype stack is now loaded with import() at the point of use instead of at server boot (~16 MiB RSS previously paid by every deployment, including processes that never save a page). Extraction runs only on the drain timer, off the request path. Guarded by no-eager-markdown-imports.spec.ts.
  • Dropped the two PageLink indexes no query uses (fromPage is already the compound index's prefix; toPath has no query until B4). Unreleased collection, so no migration.

Fixes / cleanup

  • The read endpoint no longer forwards a raw Error to apiv3Err, which was leaking the driver message into the response body; it answers an ErrorV3 with a stable code instead.
  • The Backlinks tab is hidden from share-link viewers: a share link grants one page, not the link graph around it. This is UI-only, not a security boundary — the endpoint serves guests (and share-link viewers) too, filtered to sources the requester can read, so backlinks stay readable on a public wiki.
  • Split PageLinkService by responsibility: find-backlinks.ts (read query), page-link-upsert-queue.ts (pacing, takes getSiteUrl as a callback so it needs no Crowi), and a thin Crowi-facing adapter. Public surface unchanged.
  • Removed PageLinkModel statics that were declared ahead of their B4/B5 implementations (they type-checked and threw at runtime) and the unused link-target types.
  • Documented GET /api/v3/page/backlinks in the apiv3 OpenAPI spec, and added the features/backlinks glob that generate-spec-apiv3.sh was missing.

Known gap recorded, not fixed: a recursive duplicate bulk-inserts its descendants with no per-page event, so their outbound links are never extracted. Not fixable inside this feature (the duplicate event fires before the copies exist) and Elasticsearch has the same blind spot, so it is tracked as task B3.5 to be decided in PageService for search and backlinks together.


Review fixes

Addressing @miya's review of 2026-07-31. Every numbered item is closed; the ones not listed below (#1 pino argument order, #2 the B2.2 Done-when, #3 stale design.md claims, #5 the share-link rationale, #7 the self-thrown Error in onUpsert) were already fixed by earlier commits on the branch — #5's wording also corrected in this description above.

Item Commit What changed
#6 handlePageUpsert export is test-only 92fe389808 Made private — the only production caller is handlePageUpsertById in the same file, and nothing planned reuses the document-taking shape (B3.2's backfill builds its own {path→_id} map + bulkWrite sink; B3.5 goes through the queue by id; B4.2/B5.2 are reconcile ops). The integ tests moved onto handlePageUpsertById, which also removes the duplication the review noted. With the sole caller reading the page via a projection, loadBody's isPopulated branch was unreachable and is gone.
#8 throughput hardcoded at 3/sec af89ee689e, 2e6bcf31da backlinks:drainIntervalMs / backlinks:maxPagesPerDrain (BACKLINKS_DRAIN_INTERVAL_MS / BACKLINKS_MAX_PAGES_PER_DRAIN, defaults 1000 / 3), following the app:vaultDrift* precedent. The queue takes the budget as constructor input rather than reading config, so it stays Crowi-free and CONFIG_DEFINITIONS remains the only home for the defaults. Read once at construction: unlike app:siteUrl (admin-editable, still a per-drain callback) these are env-only.
#9 [...set].slice(0, N) copies the whole queue every tick 066d548980 Batch taken by iterating with an early break — Set iteration is insertion-ordered, so the batch and its FIFO order are unchanged. This also made a malformed env var dangerous (batch.length >= NaN never breaks, so one tick would drain everything), so resolveUpsertQueuePacing now validates both values at the config boundary, falling back per value to the CONFIG_DEFINITIONS default and logging what it ignored.
#10 a hung drain wedges the queue forever 701b4debab Accepted and documented at the guard and in tasks.md B2.2, as the review allowed. A timeout cannot cancel the abandoned run — it would only race it, and a resurrected stale run would overwrite a newer link set (the upsert is last-writer-wins). Recovery is a restart; rows are then repaired by each page's next save or by B3.
#11 buildPipeline re-imports nine modules per call 5c4c8b520b Load promise memoized in a module variable; a rejection is deliberately not cached so one transient failure cannot disable extraction for the process's lifetime. The processor stays per call: both relative-link plugins are configured with the page's own path, so a shared processor would resolve every later page's relative links against the first page it was built for. Every existing case in the extractor spec used the same pagePath, so that mistake passed the whole suite — a two-page case now guards it.
#4 dropped indexes survive on existing collections ccc893f0dc autoIndex only ever creates, so fromPage_1 / toPath_1 remain on any collection already built with them. Harmless while PageLink is unreleased (hence still no migration), and Migration Strategy now records the rule that follows: once the collection has shipped, an index removal needs a migrate-mongo migration with an explicit dropIndex.
Test review — no coverage of the queue path for trash a4b13976dd The STATUS_DELETED guard was only exercised by calling handlePageUpsertById directly. Now driven through the wired service: emit create, soft-delete the page in place, assert no rows and no phantom backlink. A second page enqueued in the same tick window supplies the positive signal to wait on, so the absence assertion cannot pass merely because the drain had not run yet.

On the review's note that the pacing constants were imported from the implementation and therefore not pinned: the queue spec no longer imports anything from the implementation (it configures 500 ms / 2 pages, deliberately not the defaults, so the assertions only hold if the configured values are honoured), and the shipped defaults are pinned as literals in config-definition.spec.ts. That is stricter than the suggested upper bound — a change to 60000 fails exactly, not just out of range.

Each new guard was mutation-checked; the mutation and its single resulting failure are recorded in the commit that adds it.

Drift spec mutation evidence

Required by apps/app/.claude/rules/server-boot-imports.md. Re-adding a top-level import { unified } from 'unified' to extract-internal-link-paths.ts fails both walks with the real chain, while the two entrypoint-existence guards stay green (4 tests | 2 failed):

❯ src/features/backlinks/server/services/no-eager-markdown-imports.spec.ts (4 tests | 2 failed)
  × lazy-load boundary … has no static import chain from the backlinks server entry to the unified / remark / rehype stack
    → The backlinks server entry must not statically reach the markdown pipeline.

      features/backlinks/server/services/page-link-service.ts
        -> features/backlinks/server/services/page-link-upsert-queue.ts
        -> features/backlinks/server/services/page-link-service-handlers.ts
        -> features/backlinks/server/services/extract-internal-link-paths.ts
        => unified: expected [ Array(1) ] to deeply equal []

  ✓ lazy-load boundary … still finds the backlinks entrypoint it traces from
  × boot-time import boundary … has no static import chain from a boot entrypoint to the unified / remark / rehype stack
    → Boot entrypoints must not statically reach the markdown pipeline.

      server/crowi/index.ts
        -> features/backlinks/server/services/page-link-service.ts
        -> features/backlinks/server/services/page-link-upsert-queue.ts
        -> features/backlinks/server/services/page-link-service-handlers.ts
        -> features/backlinks/server/services/extract-internal-link-paths.ts
        => unified: expected [ Array(1) ] to deeply equal []

  ✓ boot-time import boundary … still finds every boot entrypoint it traces from

Reverted; pnpm vitest run no-eager-markdown-imports is green (4 passed).

arvid-e and others added 3 commits July 29, 2026 07:34
The coalescing queue holds page ids, so the drain re-reads the page from
the database (handlePageUpsertById) instead of using the emitted document.
The fixture assigned page.revision in memory only, leaving the drain to
extract from the previous revision, so no outbound rows were written and
the five lifecycle tests timed out.

PageService commits the revision via pushRevision before emitting, so
persisting the pointer makes the fixture match production.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A soft delete rewrites the page document in place and keeps its _id, so the
drain's existing "page is gone" check does not catch it: handlePageUpsertById
still found the page and indexed a source now sitting under /trash, leaving
rows a reader could surface as phantom backlinks.

Coalescing is what opened this window. Before it the upsert ran synchronously
inside the create/update callback, so a delete could not race it.

Fetch status alongside the body and decline to index a deleted page. Keyed on
STATUS_DELETED rather than STATUS_PUBLISHED because a legacy page carries a
null status, which GROWI treats as published; a test pins that so the guard is
not later tightened into un-indexing those pages.

Verified by mutation check: with the guard removed the new test fails with the
row it would have written.

This does not complete B2.2. Clearing the rows a page already owned when it was
trashed is reconciliation (B5.2), which is still interface-only, so the spec's
"delete supersedes a pending upsert" criterion remains unmet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@arvid-e
arvid-e requested a review from miya July 29, 2026 08:02
@mergify

mergify Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@arvid-e
arvid-e marked this pull request as draft July 29, 2026 08:43
@arvid-e
arvid-e marked this pull request as ready for review July 29, 2026 08:44
arvid-e and others added 9 commits July 30, 2026 09:40
extract-internal-links.ts was statically reachable from crowi/index.ts
(-> PageLinkService -> handlers), so the unified / remark / rehype stack
loaded at every server start. Measured in the built artifact, it adds
~16 MiB RSS on top of the rest of the boot graph (31 MiB in isolation;
~15 MiB of that is shared with mongoose / the page model and paid
regardless), and every deployment paid it forever -- including processes
that never save a page, such as the server:ci boot check.

Extraction only runs on the backlinks drain timer, off the request path,
so the stack is now loaded with dynamic import() at the point of use: the
~80 ms first load lands inside a detached timer nobody waits on, and
Node's module cache makes later extractions ~0.04 ms. The local
~/services/renderer plugins move too -- relative-links and
relative-links-by-pukiwiki-like-linker each reach hast-util-select
independently, so leaving either static would keep part of the stack
eager.

Boot chains into the pipeline: 8 -> 0.

Guarded by no-eager-markdown-imports.spec.ts, which walks two roots per
.claude/rules/server-boot-imports.md. Mutation-checked before commit:
a static 'unified' import in extract-internal-links.ts turns both walks
RED; a static 'remark-parse' import in the boot-reachable route file
turns only the boot walk RED (the route is not reachable from the
backlinks entry), which is why both roots are needed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t types

PageLinkModel declared reResolveByToPath and reconcileDeletedPages ahead
of their B4/B5 implementations. A declaration without an implementation
is worse than no declaration: PageLink.reResolveByToPath('/x') type-checks
and then throws "is not a function" at runtime. Declare each static when
the schema actually implements it.

ILinkTarget and LinkTargetState had no references anywhere in src/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GET /api/v3/page/backlinks had no annotation, so it was absent from the
generated spec while every sibling /page/* route documents itself.

The annotation alone would not have been enough: generate-spec-apiv3.sh
lists feature route globs one by one and features/backlinks was not among
them, so swagger-jsdoc would never have read the block. Add the glob too.

Uses @Swagger, matching the 53 existing annotations in this codebase (the
@openapi form in AGENTS.md is not used anywhere).

Verified with `pnpm run lint:openapi:apiv3`: spec validates, and
/page/backlinks appears in the output with its ObjectId $ref resolving.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… B3.5

Found while reviewing B2.2. A recursive duplicate bulk-inserts the copied
descendants (PageService.duplicateDescendants -> Page.insertMany) with no
per-page event, so their outbound links are never extracted. The
duplicated root is already covered -- it goes through PageService.create,
which emits 'create'.

Recorded rather than fixed because it is not fixable inside this feature:
'duplicate' carries the source page and fires before
duplicateDescendantsWithStream runs, so the copies do not yet exist and
their ids are never published in any event. The Elasticsearch index has
the same blind spot, so the fix belongs in PageService and should be
decided for search and backlinks together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… endpoint

apiv3Err turns a raw Error into ErrorV3(err.message), and `message` is an
enumerable own property, so `res.apiv3Err(err, 500)` put driver and query
internals in the response body. Verified by mutation: a rejected
findBacklinks answered

  {"errors":[{"code":null,"message":"E11000 duplicate key error
  collection: growi.pagelinks"}]}

Answer with an ErrorV3 carrying a stable code instead, keep the detail in
the server log, and add the missing `return` so the catch matches the other
exit paths. Same treatment for the 400 type guard.

The pre-existing "answers 500" test stays green under that mutation, so it
could not have caught this; the new assertion checks what must be *absent*
from the body.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the service

PageLinkService held two unrelated responsibilities that shared nothing but
the Crowi reference: the permission-filtered read query, and the coalescing
write-side queue with its timer state.

Split by responsibility domain, per .claude/rules/coding-style.md:

- find-backlinks.ts       — the read query, no Crowi needed
- page-link-upsert-queue.ts — the pacing queue; takes getSiteUrl as a
  callback rather than a Crowi, so it is unit-testable on its own
- page-link-service.ts    — thin adapter keeping only what needs a Crowi
  (event subscription, config access)

Behaviour-preserving: the public surface (PageLinkService.create,
findBacklinks) is unchanged, so the route needs no edit and all 8 existing
queue tests pass untouched apart from moving the DRAIN_INTERVAL_MS /
MAX_PAGES_PER_DRAIN import. The lifecycle integration test, which drives
real events through the queue to the database and back, also passes
unchanged.

no-eager-markdown-imports.spec.ts was re-mutation-checked after the move:
both walks still go RED, now reporting the chain through
page-link-upsert-queue.ts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fromPage carried its own index while also being the prefix of the unique
{ fromPage, toPath } compound index, which already serves every
fromPage-only lookup — replaceOutboundLinks' per-row upsert filter and its
`toPath: { $nin }` delete included. Pure write and storage overhead.

toPath's standalone index has no query at all yet. B4's re-resolve-by-path
introduces one and should add the index alongside it; the compound cannot
serve toPath alone, since it is the wrong prefix.

Remaining indexes are exactly the two that are used: the unique compound,
and toPage for findBacklinkSources. The collection is unreleased, so no
migration is needed to drop the old ones (a deployment that ran a
pre-release build keeps a harmless unused fromPage_1 until reindexed).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A share-link request carries no user and no isSharedPage flag, so
GET /page/backlinks 403s for it and the panel could only ever render its
error state. The tab and its dropdown entry were offered anyway.

Gate on isSharedUser only, not isGuestUser: the endpoint deliberately
admits guests so backlinks stay readable on a public wiki (asserted in
backlinks.spec.ts). Mirrors how the History tab guards itself —
isLinkEnabled on the modal mapping, disabled on the dropdown item.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… criterion to B5.2

B2.2 dropped the standalone {fromPage} and {toPath} indexes, but two places
still described four. B2.1 told the benchmark to check indexes "in
particular {toPage}", which would read their absence as a missing-index
finding; it now names the two that ship and why the others are gone.
B2.2's own rationale keeps its argument but stops enumerating a stale set.

B2.2 was checked off while claiming "a delete during a pending upsert
results in reconcile, not a re-created row". What it ships is narrower: the
drain re-checks status and declines a page that is now STATUS_DELETED, which
closes the window coalescing opened but leaves the rows the page already
owned when it was trashed. Its criterion now states that, and the reconcile
half moves to B5.2 (with req 3.5 added to its trace). The queue-side half —
dropping the id from the dirty set — is listed under B5.3 instead, since it
needs the PageLinkService boundary that task owns, not page-link-sync.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
arvid-e added a commit that referenced this pull request Jul 31, 2026
Requirement 3.4 asked for evidence that a heavily-linked hub page's backlinks
come back in interactive time. Measured: median 127 ms against 100,001 pages /
205,000 link rows with a 5,000-inbound hub, ~8x under the 1 s target. Raising the
hub to 20,000 inbound costs only ~1.5x (189 ms), so the shape is sub-linear and
the budget is not at risk at any realistic scale.

Both read queries are index-backed with no COLLSCAN: the distinct rides toPage_1,
the viewer filter rides _id_, and the save-path delete filter ({fromPage, toPath:
{$nin}}) rides fromPage_1_toPath_1 — so the two indexes B2.2 kept are sufficient
and no index/query fix falls out of this task. The no-rescan guarantee is checked
directly: one syncOutboundLinks issues exactly one bulkWrite whose every operation
filter is scoped to the edited fromPage, a sibling page's rows are byte-identical
afterwards, and the collection total moves only by that page's delta.

Written as an env-gated integ test rather than a CI test or a standalone script
(the decision B2.1 asked to make first). It is collected by the app-integration
project but skipped unless BACKLINKS_PERF is set, so CI never pays the 5 s seed,
while the measurement stays in-tree and reuses the crowi harness — it calls the
real findBacklinks instead of a hand-rolled copy that could drift. It expects a
real MongoDB via MONGO_URI; the default in-memory server would produce numbers
that say nothing about a deployment. The harness rewrites the db name to
growi_test_<workerId>, so the dev database is never touched, and the seed is
deleted by id in afterAll.

The seed also asserts the result set (4,000 of 5,000 sources visible under a mixed
grant/status distribution), so it doubles as a correctness check at a scale the
other integration tests do not reach.

Recorded in tasks.md with the reproduction command, the environment, and one
observation left deliberately unactioned: the distinct is FETCH <- IXSCAN rather
than a covered DISTINCT_SCAN, because its key (fromPage) is not in the index it
rides. A {toPage, fromPage} compound would cover it, but that is 10% of a read
with 8x headroom and would re-add per-save write cost B2.2 just removed.

Depends on B2.2 (#11611): the index assertions encode the two-index state that
branch introduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@miya

miya commented Jul 31, 2026

Copy link
Copy Markdown
Member

PR #11611 レビュー: feat: Coalesce link extraction


概要

タイトルは B2.2(ライブ抽出の coalescing)ですが、実際には backlinks 機能の複数の改善がまとまっています。

commit 内容
4ed4c89d f1593d7f ac0e4468 本題: PageLinkUpsertQueueSet<pageId> + paced drain)導入、trashed ソースのスキップ
8138e06c markdown pipeline(unified/remark/rehype)を dynamic import 化 + drift spec
b53aeaf3 500 レスポンスの内部エラーメッセージ漏洩を修正
962510ab 未実装 static 宣言 / 未使用の ILinkTarget 型を削除
03a9eda1 クエリが使わない {fromPage} {toPath} index を削除
dc817f23 apiv3 OpenAPI にエンドポイントを記載
5db9c826 share-link 閲覧者から Backlinks タブを隠す
c9edab4b B3.5(recursive duplicate の descendants)を spec に起票

良かった点

  • キュー設計が堅い: id を持って drain 時に最新 body を再読込、バッチを処理前に Set から除去(drain 中の save を取りこぼさない)、drainTimer/draining の二重ガード、unref()。in-flight 中の再 save が新しい run を得ることまでテストで押さえている。
  • 責務分割: find-backlinks.tspage-link-upsert-queue.ts に切り出し、PageLinkService は Crowi 依存の配線のみ。getSiteUrl をコールバック注入してキューを Crowi 非依存にしているのは coding-style の「Factory / 純粋化」方針そのもの。
  • drift spec が本物: no-eager-markdown-imports.spec.ts に対してレビュー時に mutation check を実施。extract-internal-links.tsimport { unified } from 'unified' を再導入 → 2 walk とも RED、チェーンも server/crowi/index.ts -> … -> extract-internal-links.ts => unified と正しく出る。vacuous ではない。
  • 未実装 static の型宣言を消した判断(page-link.ts)とその理由コメントは正しい。
  • テスト: unit 52 passed / page-link-service-handlers.integ 11 passed をローカルで確認。

🔴 Must fix

1. 500 のエラーが「クライアントにも出ない、ログにも出ない」

apps/app/src/features/backlinks/server/routes/backlinks.ts:37

logger.error('Failed to get backlinks', err);   // ← err が捨てられる

GROWI のロガーは pinopackages/loggerpino.child())で、シグネチャは logger.error(obj, msg)msg 先頭で第 2 引数を渡すと、placeholder が無いので err は出力に一切現れない

実証:

  • テスト実行時の出力: ERROR growi:routes:apiv3:backlink: Failed to get backlinks のみ(モックした E11000 duplicate key error collection: growi.pagelinks が消えている)
  • pino 直接検証:
{"level":50,...,"msg":"Failed to get backlinks"}                       ← 現状(err 消滅)
{"level":50,...,"err":{...stack...},"msg":"Failed to get backlinks"}   ← object-first

この行自体は既存だが、本 PR がレスポンスからエラーを外したことで、この行が唯一の診断手段になったためセットで直す必要がある。同 feature の page-link-service.ts:55logger.error({ err, pageId }, '…') と正しい順序なので、そちらに揃える。

logger.error({ err }, 'Failed to get backlinks');

2. spec と実装の乖離 — B2.2 の Done-when が未達のまま [x]

.kiro/specs/backlinks/tasks.md の B2.2 は次を要求している。

A delete-family event removes the id from the dirty set and routes to reconcileDeletedPages(delete supersedes a pending upsert)

Done when: … a delete during a pending upsert results in reconcile, not a re-created row.

実装は delete 系イベントを購読しておらず、代わりに page-link-service-handlers.ts:55-60 の防御(page == null / STATUS_DELETED で skip)に置き換えている。

  • 「orphan row を作らない」→ 満たしている
  • 「reconcile する」「dirty set から除去する」→ 満たしていない(ページが trash に落ちた場合、既存行はそのまま残る。コメントも「B5.2 で」と認めている)

判断自体は妥当(reconcileDeletedPages は B5 なのでこの PR で実装するのは越境)だが、Done-when を満たさないまま [x] を付けるのは事故のもと。tasks.md B2.2 の該当行を「B5.2 に委譲し、B2.2 では stale upsert のガードのみ」に改訂し、design.md L462-464 の "Delete supersedes a pending upsert" にも同じ注記を入れる。

3. design.md の記述が実装と食い違ったまま

  • design.md L454-455: _Implementation status — as of B1 the upsert runs inline in the event callback; the coalescing queue described here is the B2.2 target and **is not yet implemented**._ → 実装済みなので削除
  • design.md L300(3.4 のトレーサビリティ): indexes {toPage},{fromPage},{toPath}03a9eda1 で単独 index 2 本を削除したので不一致
  • tasks.md B2.2 の Why 節: 「each component write maintaining all four pagelinks indexes({fromPage}, {toPath}, {toPage}, unique {fromPage, toPath})」→ 2 本になった

🟡 Should fix

4. index 削除は既存 DB には効かない

mongoose の autoIndex作成のみで、schema から消しても既存コレクションの fromPage_1 / toPath_1 は残る。未リリース機能なので実害は薄いが、このブランチを一度でも起動した dev 環境には残留する。リリース前に migration を入れるか、「autoIndex では削除されない」ことを spec の Migration Strategy に明記する。

5. share-link のコメントが不正確

PageAccessoriesModal.tsx:75-78 / GrowiContextualSubNavigation.tsx:60-62 の「the endpoint 403s for that request」だが、login-required.ts

if (isGuestAllowed && crowi.aclService.isGuestAllowedToRead()) return next();   // ← 先に通る
if (isGuestAllowed && req.isSharedPage) return next();

の順なので、ゲスト閲覧を許可している wiki では share-link 閲覧者でも 403 にならず 200(公開ページのみ)が返る。403 になるのはゲスト閲覧が無効な場合だけ。

タブを塞ぐ判断自体は保守的で妥当だが、根拠の記述を「share-link 閲覧者にはリンクグラフを見せない方針」等に直す(コメントが load-bearing な設計判断の記録になっているため)。

なお nav の disabled と modal の isLinkEnabled の二重定義は History / ShareLink と同じ既存パターンなので、重複としては許容範囲。

6. handlePageUpsert の export が実質テスト専用

prod 経路では同ファイルの handlePageUpsertById からしか呼ばれない。export を残すなら「B3 backfill が再利用する」等の理由をコメントで残すか、integ テストを handlePageUpsertById 経由に寄せて非公開化を検討する。

7. onUpsert の自作 throw → 同関数 catch

page-link-service.ts:47-57

if (page._id == null) { throw new Error('Page ID is undefined'); }

同じ関数の catch で拾っているだけなので logger.error(…); return; で十分。PageDocument._id は実質必ず存在するので、この分岐は defensive dead code 寄りでもある(テストも無い)。


🟠 パフォーマンス

8. スループット上限が 3 pages/sec、しかもハードコード

DRAIN_INTERVAL_MS = 1000 / MAX_PAGES_PER_DRAIN = 3。design.md は「The tick cadence / batch size is the duty-cycle lever, mirroring the backfill job」と言っているが、backfill 側は cron 設定で調整可能なのに対しこちらは定数。

API から一括作成する運用(スクリプトで数百〜数千ページ)では index が数分〜数十分遅れる。最終的には追いつく(キューは FIFO で starvation なし)ので機能的には問題ないが、

  • 値の選定根拠をコメントで残す、または
  • configManager 経由にする

のどちらかを推奨。少なくとも「なぜ 3 か」がコードにも spec にも無いのは気になる。

9. [...this.pagesToUpsert].slice(0, MAX) が毎 tick で Set 全体を配列化

page-link-upsert-queue.ts:58

キューが深いとき(バースト中=まさにこの機構が効くべき局面)に、3 件取るために毎秒 N 要素の配列を確保する。バースト制御が目的のコードとしては矛盾気味。

const batch: string[] = [];
for (const id of this.pagesToUpsert) {
  batch.push(id);
  if (batch.length >= MAX_PAGES_PER_DRAIN) break;
}

10. drain がハングするとキューが永久に wedge する

handlePageUpsertById が resolve しないと drainingtrue のままで、以降 scheduleDrain() が永久に早期 return する(enqueue は Set に積むだけ)。タイムアウトは無いので、割り切るなら既知の制約としてコメントを残す。

11. buildPipeline は毎回 9 本の dynamic import + processor 構築

2 回目以降は module registry 解決(コメント通り ~0.04ms)なので大きくないが、Promise.all 自体は毎回走る。module 変数に load promise をメモ化すれば消せる(任意)。lazy 化の判断とコメントの説明は非常に良いので、これは nit。


✅ セキュリティ

  • エラーメッセージ漏洩の修正は正しい。テストも JSON.stringify(res.body)).not.toContain('E11000') と実装非依存で良い contract test。
  • pageIdisMongoId() で検証済み、regex 生成なし → .claude/rules/mongodb-regex.md の抵触なし(target-page-resolution.ts / page-link-sync.ts にも RegExp / $regex なしを確認)。
  • 読み取りは addViewerCondition + addConditionToExcludeTrashed、DTO は _id/path のみ → grant 準拠。
  • 抽出は grant を無視して index するが read 時にフィルタする設計(design 通り)なので情報漏洩なし。
  • 残課題は update permissions (755 to 644) #1(可観測性の喪失)Imprv/dev arch #5(コメントの記述) のみ。

🧪 テストレビュー(essential-test-design / essential-test-patterns 適用)

良い

  • page-link-service.spec.ts: advanceTimersByTimeAsync(DRAIN_INTERVAL_MS - 1) で未呼び出し → +1 で呼び出し、という境界アサーションは skill が推奨する形そのもの。in-flight 中の別ページ save / 同一ページ save / 1 件失敗しても残りを処理 / 失敗後も詰まらない、と契約を網羅している。upsertedIds().sort() で「順序は契約でないが取りこぼしも重複もない」を表現しているのも良い。
  • drift spec は rule(apps/app/.claude/rules/server-boot-imports.md)が要求する「2 つの root から walk」「entrypoint 存在ガード」を満たす。mutation check はレビュー時に実施して RED を確認済みだが、rule は「red 出力を PR の evidence に含める」ことを要求しているので PR 本文に貼る。
  • page-link-service-handlers.integ.ts: mock は resolveToPages(境界)のみで、あとは実 DB の観測可能状態でアサート。legacy な status 未設定ケース($unset)まで見ているのは良い。
  • mock<PageDocument> + page._id = pageId は Tier 2 の局所回避として妥当、理由コメントもある。

気になる

  • DRAIN_INTERVAL_MS / MAX_PAGES_PER_DRAIN を実装から import してアサートしているため、値そのものは pin されていない。 誰かが DRAIN_INTERVAL_MS60000 に変えても全テストが緑のまま。「バーストを 1 tick に詰め込まない」は守れるが「ユーザーが体感する遅延」は守れない — anti-pattern 1(implementation spy)の変種。上限だけリテラルで固定すると degrade を検知できる。

    it('paces within a user-tolerable window', () => {
      expect(DRAIN_INTERVAL_MS).toBeLessThanOrEqual(5000);
    });
  • Omit symlinks #2 と対になるテストが無い: 「pending upsert 中に delete/trash された」ケースの統合テストがない(handlePageUpsertById 単体の trashed テストはあるが、キュー経由の経路は未カバー)。

  • handlePageUpsert (integration) の既存 describe と新 describe で「抽出 → 行同期」のアサートがかなり重複している。新規側は「id で引く/最新 body/存在しない/trashed/legacy status」に絞れているので許容範囲だが、Feature plugin2 #6 で非公開化するなら統合の余地がある。

  • PageModelFactory(null) は戻り値が any(model 側の問題で本 PR の責任ではない)。Page: PageModel に代入しているので実害なし。

  • page-link-lifecycle.integ.tsPage.updateOne 追加は正しい修正(drain が DB から再読込するため)で、コメントも理由を書いている。vi.waitFor(timeout: 15000) なので 1s の drain でも成立する。


その他

  • features/backlinks/server/routes/ は他機能の server/routes/apiv3/ と階層が違い、openapi の glob もそこだけ特例(server/routes/*.ts)になっている。統一するなら今のうち。
  • swagger ブロックのインデントが 4 スペース(他機能は 2)。生成は通る — swagger-jsdoc を実行して /page/backlinks が出力され、$ref: ObjectId も解決することを確認済み。
  • swagger に 403 レスポンスの記載が無い(ゲスト不許可 / share-link 時)。security は global に fallback があり他の /page/* も個別指定していないので、こちらは揃っている。
  • PR 本文が Redmine リンク 2 本のみ。スコープがタイトルより広いので、何をやったかの要約と、drift spec の mutation evidence を書く。

実施した検証

内容 結果
unit(no-eager-markdown-imports / page-link-service.spec / backlinks.spec / extract-internal-links / backlinks.spec.tsx 52 passed
integ(page-link-service-handlers.integ 11 passed
drift spec の mutation check RED を確認 → revert 済み(working tree クリーン)
OpenAPI 生成 成功、/page/backlinks 出力を確認
pino の引数順(#1 実証済み
biome check(backlinks 配下) 新規指摘なし(既存の noDefaultExport 1 件のみ)
page-link-lifecycle.integ / page-link-service.integ ローカル実行不可page-bulk-exportbulk-export.generated 未生成のため(本 PR と無関係)。CI での結果確認が必要

arvid-e and others added 10 commits August 4, 2026 08:41
…eturn

`extractInternalLinks` returns resolved page paths, not link nodes, and
`resolveToPages` returns a Map of page ids rather than page documents — both
names invited the wrong mental model at the call site. Rename to
`extractInternalLinkPaths` (file: extract-internal-link-paths.ts) and
`resolveToPageIds`, and update the handlers and specs that reference them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Names: the renamed `extractInternalLinkPaths` / `resolveToPageIds` throughout,
`getBacklinksHandlerFactory (routes/backlinks.ts)` for the read endpoint, and
the batched resolver contract (paths -> Map, unresolved inputs absent from the
map) in place of the per-link `toPath -> id | null` signature that was never
implemented. Extraction is documented as async, with the reason (the markdown
stack is lazy-loaded to keep it out of the boot graph).

File Structure Plan: reconciled against the tree — services/ (not service/),
the split interfaces/, the three modules the plan never listed
(page-link-service-handlers, page-link-upsert-queue, find-backlinks), and
story markers on what is not built yet (B3 backfill, B4.2/B5.2 sync ops,
B5.5 badge, B5.6 forward-health section).

`ILinkTarget` and `LinkTargetState`: B1.1 planned to declare them up front but
B1 shipped without them, so they now belong to the tasks that produce them —
the union to B5.1, the DTO to B5.4. Target shapes are unchanged; 6.4 stays
covered by B1.8 and B5.4-B5.7.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y loading

Review #6 of PR #11611: handlePageUpsert was exported but had exactly one
production caller, handlePageUpsertById in the same file. Nothing planned wants
the document-taking shape either — B3.2's backfill builds its own {path->_id}
map and bulkWrite sink, B3.5 goes through the queue by id, and B4.2/B5.2 are
reconcile ops — so the export was effectively test-only.

Since the sole caller reads the page with a projection and never populates, the
revision is always a ref there: loadBody's isPopulated branch was unreachable
and is dropped with it.

The integ tests move to handlePageUpsertById and its existing Page/Revision
fixtures, which also collapses the extraction/row-sync duplication the reviewer
noted between the two describes. The "unpopulated event payload" case goes away
with the branch it described — a leftover from the pre-coalescing design where
the handler took the emitted document.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The drain interval and per-tick page budget were hardcoded constants in
page-link-upsert-queue.ts, so the duty cycle that bounds live extraction could
not be tuned per deployment. Add them as env-only config, following the
app:vaultDrift* precedent:

  backlinks:drainIntervalMs    BACKLINKS_DRAIN_INTERVAL_MS     default 1000
  backlinks:maxPagesPerDrain   BACKLINKS_MAX_PAGES_PER_DRAIN   default 3

The queue receives the pair as constructor input rather than reading config
itself, so it stays Crowi-free and unit-testable and CONFIG_DEFINITIONS remains
the only place the defaults live. PageLinkService reads them once at
construction: unlike app:siteUrl, which admins can change at runtime and which
is therefore still a per-drain callback, these are env-only. configManager is
loaded at crowi/index.ts:275, long before PageLinkService.create.

The queue spec now configures 500 ms / 2 pages — deliberately not the shipped
defaults, so the pacing assertions fail if the values are ignored (verified:
re-hardcoding 1000/3 turns 7 of its 8 tests red).

Note: constructing PageLinkService now requires a loaded configManager, so
page-link-service.integ.ts can no longer pass a bare mock<Crowi>().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
B2.2's duty cycle is now env-tunable rather than a pair of constants, so state
the keys, env vars and defaults where design.md and tasks.md describe the lever.
Also name handlePageUpsertById as the per-page unit, now that handlePageUpsert
is private.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review #9 of PR #11611: `[...this.pagesToUpsert].slice(0, max)` allocated an
array of the entire dirty set every tick just to read `max` ids from it, so the
per-tick cost scaled with queue depth. A deep queue only happens during a burst,
which is the one situation this queue exists to bound — take the batch by
iterating with an early break instead. Set iteration is insertion-ordered, so
the batch and its FIFO order are unchanged.

The loop trusts the budget in a way `slice` did not: `batch.length >= NaN` is
never true, so it would never break and one tick would parse the whole queue.
That is reachable now that the value comes from an env var, since config-loader
parses numeric env vars with a bare `parseInt` and never validates the result —
BACKLINKS_MAX_PAGES_PER_DRAIN=three would silently disable the pacing outright.
`resolveUpsertQueuePacing` therefore validates both values at the config
boundary, falling back per value to the default declared in CONFIG_DEFINITIONS
(read from there rather than restated, so the defaults stay single-sourced) and
logging what it ignored. Zero and negative budgets are rejected the same way —
with those the queue would never drain at all.

Tested at the contract level rather than only on the resolver: the queue spec
configures NaN and asserts a tick still drains the declared default, so an
unwired guard fails there (verified: it is the only failure when the
resolveUpsertQueuePacing call is removed). The warning has its own assertion —
falling back silently would leave an operator with no way to find the typo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review #11 of PR #11611: buildPipeline re-ran nine `import()`s and their
Promise.all on every extraction. Node caches the modules, so the repeat cost is
registry lookups rather than parses, but the await plumbing is pure waste — cache
the load promise in a module variable instead. A rejected promise is deliberately
not cached: one transient load failure would otherwise disable extraction for the
lifetime of the process.

The processor stays per call. Both relative-link plugins are configured with the
page's own path, so a shared processor would resolve every later page's relative
links against the first page it was built for. Every existing case in the spec
used the same pagePath, so that mistake would have gone unnoticed — a case that
extracts from two different pages now covers it (verified: memoizing the whole
pipeline fails it and nothing else).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
arvid-e and others added 3 commits August 5, 2026 05:32
Review #10 of PR #11611: an upsert that never settles leaves `draining` true, so
scheduleDrain early-returns for the rest of the process's life while enqueue keeps
filling the set — the instance silently stops indexing.

Accepted rather than guarded, as the reviewer allowed, and the reason is recorded
at the guard and in the spec: a timeout cannot cancel the abandoned run, so it
would only race it, and a resurrected stale run would overwrite a newer link set
(the upsert is last-writer-wins). Recovery is a restart, after which each page's
rows are repaired by its next save or by the B3 backfill — the same path as any
other dropped work in this best-effort queue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of PR #11611 (test section): the drain's STATUS_DELETED guard was only
covered by calling handlePageUpsertById directly — the path that actually
matters, a create event landing in the queue and the page being trashed before
the drain reads it, had no test.

Drives it through the wired service: emit create, soft-delete the page in place
(path + status, as GROWI does, so the id stays resolvable and the drain still
finds it), then assert no rows and no phantom backlink on the target. A second
page enqueued in the same tick window supplies the positive signal to wait on —
polling for its row proves the batch drained, so the absence assertion cannot
pass just because the drain had not run yet.

Verified by mutation: dropping `if (page.status === Page.STATUS_DELETED) return`
fails this test and nothing else.

The afterEach cleanup now also matches /trash… paths, since a soft delete
rewrites the path out from under the seed prefix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review #4 of PR #11611: B2.2 removed the redundant {fromPage} and {toPath} index
declarations, but Mongoose autoIndex only ever creates — fromPage_1 / toPath_1
survive on any collection already built with them, so dev and staging instances
that ran an earlier build keep paying their write cost.

Harmless while PageLink is unreleased, which is why no migration ships now.
Record that in Migration Strategy together with the rule it implies: once the
collection has shipped, removing an index needs a migrate-mongo migration with an
explicit dropIndex, not just a schema edit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@arvid-e
arvid-e requested a review from yuki-takei August 5, 2026 07:07
'backlinks:maxPagesPerDrain': defineConfig<number>({
envVarName: 'BACKLINKS_MAX_PAGES_PER_DRAIN',
defaultValue: 3,
}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

drain のペースが固定値というのがあまり良い戦略ではない

1万ページのインポートは 10000/3 ≈ 55分 はキューに残る。

例えば以下のような案はどうか?

案1

「N件/tick」をやめて、1件処理するのにかかった時間を測り、その時間に比例して休む。

const started = performance.now();
await handlePageUpsertById(id, siteUrl);
const workMs = performance.now() - started;
// duty = 0.2 なら「働いた時間の4倍休む」= イベントループ占有率の上限が 20%
await sleep(workMs * (1 / duty - 1));

ページサイズに自動追随する。小さいページは事実上バーストする(1秒で数十件流れる)し、巨大ページは1件ずつ・長めに休む
設定が「占有率の上限」1つに減る。

案X

別に他の案でもよい
検討してほしい

@yuki-takei

Copy link
Copy Markdown
Contributor

miya が指摘したものの中で未対応のものがある

直してほしいもの

1. 処理に失敗したページが、更新されないまま終わる

page-link-upsert-queue.tsdrain() は、その回に処理するページ ID を、処理を始める前に
キュー(pagesToUpsert)から取り除いています。そのため handlePageUpsertById が失敗すると、
エラーがログに記録されるだけで、そのページはもう一度処理されることがありません。

MongoDB 側の一時的なエラー(レプリカセットの切り替え、書き込みのタイムアウトなど)で1ページ分が
失敗すると、そのページの pagelinks の行は古い内容のまま残ります。直るのは、そのページが次に
保存されたときか、B3 の backfill(既存ページをまとめて処理する仕組み)が走ったときです。

drain が止まってしまう場合(#10)については「そういうものとして受け入れる」と、コードと
tasks.md の両方に書かれています。しかしこちらの「1ページ失敗すると、そのページだけ更新されない
まま残る」ことは、design.md にも tasks.md にも書かれていません。そのうえ、テスト
logs a failing page and still upserts the rest of the batch が、この動きを正しいものとして
固定しています。

最低限、次のどちらかをお願いします。

  • 失敗したページ ID をキューに戻す。ただし同じページで永遠に再試行し続けないように、試行回数を
    数えておき、決めた回数を超えたらそのページは諦めて、エラーとして記録する
  • キューに戻さないのであれば、store images for web #10 と同じように「一時的な失敗で更新できなかったページは、次の
    保存か backfill まで古いままになる」と、制約として書き残す

2. ページを取り出す前に例外が起きると、1件も処理しないまま同じ失敗を繰り返し続ける

drain()try の最初で this.getSiteUrl() を呼んでいます。これは、その回に処理するページ ID を
取り出すよりも前の位置です。

configManager.getConfig は、設定がまだ読み込まれていないときに Config is not loaded という
例外を投げます(config-manager.ts:82-84)。この位置で例外が起きると、処理待ちのページ ID は
1件もキューから取り除かれません。そして finally 節はキューが空でないことだけを見て、必ず次の
タイマーを設定します。結果として、1秒ごとに同じエラーを出しながら、1ページも処理しない状態が
続きます。

いまの起動順序(設定を読み込んだあとに PageLinkService を作る)であれば、この例外は実際には
起きないはずですが、構造自体はちょっと危ない。getSiteUrl() の呼び出しを、ページ ID を
取り出したあとに移してください。そうすれば、仮に例外が起きても、キューに溜まったページの処理は
進みます。

3. ゴミ箱との競合を確かめる integ テストが、タイミング次第で失敗する

page-link-lifecycle.integ.ts
does not index a page trashed while its upsert sat in the queue (3.5) は、次の順で書かれています。

  1. emitUpsert('create', trashed, …) で登録する(このとき 1000ms 後のタイマーが設定される)
  2. Page.updateOne でそのページの status を deleted に変える

CI の MongoDB が混んでいて 2 が1秒を超えると、drain() は status がまだ published のままの
ページを読み、pagelinks に行を書いてしまいます。すると
expect(await outboundRows(trashed._id)).toEqual([]) が失敗します。

比較用のページを一緒に登録することで、「実際には drain がまだ走っていないのに、行が無いことを
もってテストが通ってしまう」ほうは防げています。しかし逆向き、つまり「ゴミ箱に入れるのが
間に合わない」ほうには対策がありません。

emitUpsert を呼ぶ前にゴミ箱に入れておくか、status が deleted になったことを確認してから
比較用のページを登録すれば、タイミングに左右されなくなります。

ついでに直せるもの

他にも諸々ありそうなのでチェックしてください

今回は直さなくてよいもの

  • 数値の環境変数が NaN(数値として解釈できない値)になる問題は、設定を読み込む共通の仕組み
    parseEnvValue)側の穴です。resolveUpsertQueuePacing で backlinks 側だけ塞いだのは、
    対処として妥当だと思います。本来直すべきなのは parseEnvValue が NaN のときに既定値へ
    戻すことなので、別の issue として立てることを推奨します。
  • routes ファイルの置き場所と openapi 生成スクリプトの特例、swagger の 403 の記載。

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.

3 participants