mirror of
https://github.com/toeverything/AFFiNE.git
synced 2026-09-21 03:03:15 +08:00
fix(nbstore): accumulate overlapping doc priority requests (#15595)
## Description While reading `DocSyncPeer.addPriority` I noticed the fix from #15338 (commit `cd6593c6`) was applied to only one of the three near-identical copies of that method in `nbstore`. The two others still carry the pre-#15338 code: - `DocFrontend.addPriority` — `packages/common/nbstore/src/frontend/doc.ts` - `IndexerSyncStatus.addPriority` — `packages/common/nbstore/src/sync/indexer/index.ts` Both write the incoming priority over whatever was stored: ```ts const oldPriority = this.prioritySettings.get(id) ?? 0; this.prioritySettings.set(id, priority); // overwrites this.jobDocQueue.setPriority(id, oldPriority + priority); // adds ``` The two lines already disagree with each other, and the release callback returned by the method *subtracts* `priority` from the current value, so it only makes sense if the forward path adds. The result is that overlapping priority requests for the same doc don't stack, and releasing one of them drops the doc to zero instead of back to the level the remaining holder asked for. That overlap is reachable from the UI. `WorkspaceEngine.doc` is a `DocFrontend` (`entities/engine.ts` → `client.docFrontend`), and two call sites add +10 to the same `pageId`: `detail-page-wrapper.tsx` and `peek-view/view/utils.ts`. Open a doc, peek the same doc, close the peek view, and the still-open doc is left at priority 0 — behind every other queued doc. The indexer has the same pair: the navigation panel node and the doc-summary store both add +10 to the same `docId`. The fix is the same shape as #15338 — compute `newPriority` / `restoredPriority` once and use it for both the map and the queue. ### Test `doc priority requests accumulate` in `packages/common/nbstore/src/__tests__/frontend.spec.ts`. Two holders take +10 on `high` and one of them releases; `low` sits at +5; both load jobs are queued before `start()` so the queue priority alone decides the load order, which the test reads by spying on `storage.getDoc`. On `canary` it fails: ``` FAIL packages/common/nbstore/src/__tests__/frontend.spec.ts > doc priority requests accumulate AssertionError: expected [ 'low', 'high' ] to deeply equal [ 'high', 'low' ] ``` With the fix, the whole package is green (`yarn vitest run packages/common/nbstore`: 16 files, 86 tests). ## Checklist - [x] I have signed the [AFFiNE Contributor License Agreement](https://cla-assistant.io/toeverything/AFFiNE) - [x] The PR targets the `canary` branch and its title follows [Conventional Commits](https://www.conventionalcommits.org/) - [x] Tests are added or updated where it makes sense - [x] `yarn lint` and `yarn typecheck` pass locally - [x] If the PR code includes AI-generated edits, I have carefully reviewed it to ensure that the scope of the PR is consistent with what is claimed in the title and description, and that there are no redundant or invalid implementations. - [x] I agree that maintainers may close the PR without discussion if they consider the code quality to be too low. AI tools used <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved priority handling for document loading and indexing so multiple priority requests accumulate correctly. * Ensured queued work consistently reflects remaining priority requests after individual requests are released. * High-priority documents and indexing tasks are now processed ahead of lower-priority work as expected. * **Tests** * Added coverage validating accumulated priority behavior for document loading and index crawling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -147,3 +147,46 @@ test('awareness', async () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
test('doc priority requests accumulate', async () => {
|
||||
const docStorage = new IndexedDBDocStorage({
|
||||
id: 'ws-priority',
|
||||
flavour: 'a',
|
||||
type: 'workspace',
|
||||
});
|
||||
|
||||
docStorage.connection.connect();
|
||||
|
||||
await docStorage.connection.waitForConnected();
|
||||
|
||||
const frontend = new DocFrontend(docStorage, DocSyncImpl.dummy);
|
||||
|
||||
// two holders prioritize the same doc, then one of them goes away
|
||||
frontend.addPriority('high', 10);
|
||||
const releaseSecondHolder = frontend.addPriority('high', 10);
|
||||
releaseSecondHolder();
|
||||
|
||||
frontend.addPriority('low', 5);
|
||||
|
||||
const loadOrder: string[] = [];
|
||||
vitest.spyOn(docStorage, 'getDoc').mockImplementation(async docId => {
|
||||
loadOrder.push(docId);
|
||||
return null;
|
||||
});
|
||||
|
||||
// both load jobs are queued before the main loop starts, so the queue
|
||||
// priority alone decides which doc is loaded first
|
||||
frontend.connectDoc(new YDoc({ guid: 'low' }));
|
||||
frontend.connectDoc(new YDoc({ guid: 'high' }));
|
||||
|
||||
frontend.start();
|
||||
|
||||
await vitest.waitFor(
|
||||
() => {
|
||||
expect(loadOrder).toEqual(['high', 'low']);
|
||||
},
|
||||
{ timeout: 2000 }
|
||||
);
|
||||
|
||||
frontend.stop();
|
||||
});
|
||||
|
||||
@@ -918,3 +918,71 @@ test('indexer completion waits for the current job to finish', async () => {
|
||||
sync.stop();
|
||||
}
|
||||
});
|
||||
|
||||
test('indexer priority requests accumulate', async () => {
|
||||
const docsInRootDoc = new Map([
|
||||
['doc-low', { title: 'Doc Low' }],
|
||||
['doc-high', { title: 'Doc High' }],
|
||||
]);
|
||||
const crawled: string[] = [];
|
||||
const rootDocCrawlStarted = deferred<void>();
|
||||
const releaseRootDocCrawl = deferred<void>();
|
||||
const docStorage = new TestDocStorage(
|
||||
'workspace-id',
|
||||
new Map([
|
||||
['doc-low', new Date('2026-01-01T00:00:00.000Z')],
|
||||
['doc-high', new Date('2026-01-01T00:00:00.000Z')],
|
||||
]),
|
||||
async docId => {
|
||||
crawled.push(docId);
|
||||
return { title: docId, summary: 'summary', blocks: [] };
|
||||
}
|
||||
);
|
||||
const indexer = new TrackingIndexerStorage([], 30_000);
|
||||
// hold the loop inside the root doc crawl, so both docs stay queued while
|
||||
// their priorities are changed
|
||||
let holding = false;
|
||||
vi.spyOn(indexer, 'insert').mockImplementation(async () => {
|
||||
if (!holding) {
|
||||
holding = true;
|
||||
rootDocCrawlStarted.resolve();
|
||||
await releaseRootDocCrawl.promise;
|
||||
}
|
||||
});
|
||||
const sync = new IndexerSyncImpl(
|
||||
docStorage,
|
||||
{
|
||||
local: indexer,
|
||||
remotes: {},
|
||||
},
|
||||
new TrackingIndexerSyncStorage([])
|
||||
);
|
||||
|
||||
vi.spyOn(reader, 'readAllDocsFromRootDoc').mockImplementation(
|
||||
() => new Map(docsInRootDoc)
|
||||
);
|
||||
|
||||
try {
|
||||
sync.start();
|
||||
await rootDocCrawlStarted.promise;
|
||||
|
||||
sync.addPriority('doc-low', 5);
|
||||
// two holders on the same doc, one of them goes away
|
||||
sync.addPriority('doc-high', 10);
|
||||
const releaseSecondHolder = sync.addPriority('doc-high', 10);
|
||||
releaseSecondHolder();
|
||||
|
||||
releaseRootDocCrawl.resolve();
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(crawled).toHaveLength(2);
|
||||
});
|
||||
|
||||
// the remaining holder still asked for +10, so `doc-high` must outrank the
|
||||
// +5 of `doc-low`
|
||||
expect(crawled).toEqual(['doc-high', 'doc-low']);
|
||||
} finally {
|
||||
releaseRootDocCrawl.resolve();
|
||||
sync.stop();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -399,14 +399,16 @@ export class DocFrontend {
|
||||
addPriority(id: string, priority: number) {
|
||||
const undoSyncPriority = this.sync?.addPriority(id, priority);
|
||||
const oldPriority = this.prioritySettings.get(id) ?? 0;
|
||||
const newPriority = oldPriority + priority;
|
||||
|
||||
this.prioritySettings.set(id, priority);
|
||||
this.status.jobDocQueue.setPriority(id, oldPriority + priority);
|
||||
this.prioritySettings.set(id, newPriority);
|
||||
this.status.jobDocQueue.setPriority(id, newPriority);
|
||||
|
||||
return () => {
|
||||
const currentPriority = this.prioritySettings.get(id) ?? 0;
|
||||
this.prioritySettings.set(id, currentPriority - priority);
|
||||
this.status.jobDocQueue.setPriority(id, currentPriority - priority);
|
||||
const restoredPriority = currentPriority - priority;
|
||||
this.prioritySettings.set(id, restoredPriority);
|
||||
this.status.jobDocQueue.setPriority(id, restoredPriority);
|
||||
|
||||
undoSyncPriority?.();
|
||||
};
|
||||
|
||||
@@ -824,13 +824,15 @@ class IndexerSyncStatus {
|
||||
|
||||
addPriority(id: string, priority: number) {
|
||||
const oldPriority = this.prioritySettings.get(id) ?? 0;
|
||||
this.prioritySettings.set(id, priority);
|
||||
this.jobs.setPriority(id, oldPriority + priority);
|
||||
const newPriority = oldPriority + priority;
|
||||
this.prioritySettings.set(id, newPriority);
|
||||
this.jobs.setPriority(id, newPriority);
|
||||
|
||||
return () => {
|
||||
const currentPriority = this.prioritySettings.get(id) ?? 0;
|
||||
this.prioritySettings.set(id, currentPriority - priority);
|
||||
this.jobs.setPriority(id, currentPriority - priority);
|
||||
const restoredPriority = currentPriority - priority;
|
||||
this.prioritySettings.set(id, restoredPriority);
|
||||
this.jobs.setPriority(id, restoredPriority);
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user