Skip to content

Commit 18e5f39

Browse files
Merge pull request #33 from HarperFast/fix/pause-readback
fix(plugin): resolve pause intent from the just-written value, not a re-read; v0.8.2
2 parents d97eadb + 3bbb003 commit 18e5f39

6 files changed

Lines changed: 148 additions & 10 deletions

File tree

package-lock.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/plugin/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@harperfast/prerender",
3-
"version": "0.8.1",
3+
"version": "0.8.2",
44
"type": "module",
55
"description": "Configurable Harper plugin for prerendering pages for bots and crawlers",
66
"license": "Apache-2.0",

packages/plugin/src/resources/PrerenderAdmin.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,9 @@ async function buildNodeList(now) {
208208
: null,
209209
// Scopes a control write is allowed to name, so a typo can't create a row that
210210
// silently never applies to anything.
211-
knownScopes: [CLUSTER_SCOPE, ...new Set([...statuses.map((r) => r.hostname), ...controlByScope.keys()])].filter(
211+
// CLUSTER_SCOPE goes INSIDE the Set: a cluster control row also appears in
212+
// controlByScope, so prepending it outside listed "all" twice.
213+
knownScopes: [...new Set([CLUSTER_SCOPE, ...statuses.map((r) => r.hostname), ...controlByScope.keys()])].filter(
212214
Boolean
213215
),
214216
};

packages/plugin/src/resources/RenderQueue.js

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ const mutex = getMutex('render_queue');
2525
* this on its own status-sync interval, so a replicated intent write converges everywhere
2626
* within one `queue.statusSyncInterval`.
2727
*/
28-
async function syncQueueState(force = false) {
29-
const desired = await getDesiredPause(server.hostname);
28+
async function syncQueueState(force = false, pending = null) {
29+
const desired = await getDesiredPause(server.hostname, pending);
3030

3131
if (desired.paused) {
3232
await QueueState.reportStatus('paused');
@@ -84,10 +84,20 @@ export class RenderQueue extends Resource {
8484
* 'all' again). Remote nodes pick the change up on their next status sync.
8585
*/
8686
static setPause = mutex.withLock(async ({ scope, paused, updatedBy } = {}) => {
87-
const intent = await setDesiredPause(scope ?? server.hostname, paused, updatedBy);
87+
const target = scope ?? server.hostname;
88+
const intent = await setDesiredPause(target, paused, updatedBy);
8889
// Re-resolve rather than assuming the write applies here: a cluster-wide pause does
8990
// not pause a node carrying an explicit `paused: false` override, and vice versa.
90-
const local = await syncQueueState(true);
91+
//
92+
// The just-written scope is passed as `pending` instead of being re-read: a row
93+
// deleted earlier in this request is still visible to a read here, so re-reading it
94+
// resolves a resume straight back to "paused" and returns the opposite of what
95+
// actually happened. The other scope is read normally — this write didn't touch it.
96+
// `intent.paused`, not the raw argument: setDesiredPause normalizes (an absent `paused`
97+
// is written as `false`), so threading the raw value could resolve "no opinion" while
98+
// the row on disk says `false`. Using what was actually written keeps the resolved
99+
// state and the persisted state identical by construction.
100+
const local = await syncQueueState(true, { scope: target, paused: intent.paused });
91101
return { ...intent, node: server.hostname, local };
92102
});
93103

packages/plugin/src/util/queueControl.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ export const resolveDesiredPause = (nodeControl, clusterControl) => {
4444
* Read both intent records for `hostname` and resolve them. Kept separate from the pure
4545
* resolver above so the precedence rules stay unit-testable outside Harper.
4646
*/
47-
export const getDesiredPause = async (hostname) => {
47+
export const getDesiredPause = async (hostname, pending = null) => {
4848
const { QueueControl } = databases.render_service;
4949

5050
// `select` MUST be an array here. A string `select` projects to the bare scalar rather
@@ -53,9 +53,17 @@ export const getDesiredPause = async (hostname) => {
5353
// `.paused` off a boolean gets undefined, silently treats it as "no opinion", and every
5454
// pause resolves to "not paused". That failure is invisible: the row is written and shows
5555
// up in the UI, but no node ever acts on it.
56+
//
57+
// `pending` substitutes a scope's value instead of reading it, for the caller that has
58+
// just written that scope in this same request. A row deleted earlier in the request is
59+
// still visible to this read (the read does not observe the delete), so re-reading it
60+
// resolves a resume back to "paused" and reports the opposite of what happened. Trusting
61+
// the value just written removes the dependency on write visibility entirely.
62+
const pendingFor = (scope) => (pending && pending.scope === scope ? { paused: pending.paused } : undefined);
63+
5664
const [nodeControl, clusterControl] = await Promise.all([
57-
QueueControl.get({ id: hostname, select: ['paused'] }),
58-
QueueControl.get({ id: CLUSTER_SCOPE, select: ['paused'] }),
65+
pendingFor(hostname) ?? QueueControl.get({ id: hostname, select: ['paused'] }),
66+
pendingFor(CLUSTER_SCOPE) ?? QueueControl.get({ id: CLUSTER_SCOPE, select: ['paused'] }),
5967
]);
6068

6169
return resolveDesiredPause(nodeControl, clusterControl);

packages/plugin/test/queueControlRead.test.js

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,121 @@ test('the read projects to a RECORD, not a bare scalar', async () => {
9595
assert.ok(Array.isArray(select), `select must be an array, got ${typeof select} (${select})`);
9696
}
9797
});
98+
99+
/**
100+
* The `pending` substitution. `setPause` writes an intent and then immediately re-resolves
101+
* it in the same request — but a row deleted earlier in that request is still visible to
102+
* the read, so re-reading it resolves a resume back to "paused" and the API reports the
103+
* opposite of what happened. The fakes below return deliberately STALE rows to prove the
104+
* resolution trusts the just-written value instead of the table.
105+
*/
106+
107+
test('pending resume wins over a stale row that still reads as paused', async () => {
108+
// The delete already happened; the read just cannot see it yet.
109+
globalThis.databases = makeFakeDatabases({ 'node-a': { scope: 'node-a', paused: true } });
110+
111+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-a', paused: null }), {
112+
paused: false,
113+
source: 'default',
114+
});
115+
});
116+
117+
test('pending resume on a node still inherits a cluster pause', async () => {
118+
globalThis.databases = makeFakeDatabases({
119+
'all': { scope: 'all', paused: true },
120+
'node-a': { scope: 'node-a', paused: false },
121+
});
122+
123+
// Clearing the node override must fall back to the cluster row, not to "running".
124+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-a', paused: null }), {
125+
paused: true,
126+
source: 'cluster',
127+
});
128+
});
129+
130+
test('pending pause wins over a stale row that has not appeared yet', async () => {
131+
globalThis.databases = makeFakeDatabases({});
132+
133+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-a', paused: true }), {
134+
paused: true,
135+
source: 'node',
136+
});
137+
});
138+
139+
test('a pending cluster write leaves the node override authoritative', async () => {
140+
globalThis.databases = makeFakeDatabases({ 'node-a': { scope: 'node-a', paused: false } });
141+
142+
// Cluster-wide pause requested, but this node is explicitly pinned to keep running.
143+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'all', paused: true }), {
144+
paused: false,
145+
source: 'node',
146+
});
147+
// A node with no override does take the pending cluster pause.
148+
assert.deepEqual(await getDesiredPause('node-b', { scope: 'all', paused: true }), {
149+
paused: true,
150+
source: 'cluster',
151+
});
152+
});
153+
154+
test('pending for an unrelated scope does not shadow either read', async () => {
155+
globalThis.databases = makeFakeDatabases({ all: { scope: 'all', paused: true } });
156+
157+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-z', paused: false }), {
158+
paused: true,
159+
source: 'cluster',
160+
});
161+
});
162+
163+
/**
164+
* `setDesiredPause` normalization. `setPause` threads `intent.paused` (what was written)
165+
* rather than its raw argument into the pending substitution, so these pin the property
166+
* that makes that safe: the returned `paused` is always a boolean or null, never undefined.
167+
*/
168+
169+
test('setDesiredPause normalizes a missing paused to false, and reports what it wrote', async () => {
170+
const writes = [];
171+
globalThis.databases = {
172+
render_service: {
173+
QueueControl: {
174+
put: (scope, record) => {
175+
writes.push({ scope, ...record });
176+
return Promise.resolve();
177+
},
178+
delete: (scope) => {
179+
writes.push({ scope, deleted: true });
180+
return Promise.resolve();
181+
},
182+
},
183+
},
184+
};
185+
const { setDesiredPause } = await import('../src/util/queueControl.js');
186+
187+
// An absent `paused` is written as false — so the value threaded into `pending` must be
188+
// that same normalized false, not undefined (which would resolve as "no opinion" and
189+
// contradict the row just written).
190+
const intent = await setDesiredPause('node-a', undefined, 'tester');
191+
assert.equal(intent.paused, false);
192+
assert.equal(writes[0].paused, false);
193+
194+
// null deletes and reports null (inherit), which IS "no opinion" — correctly so.
195+
const cleared = await setDesiredPause('node-a', null, 'tester');
196+
assert.equal(cleared.paused, null);
197+
assert.equal(cleared.inherited, true);
198+
assert.equal(writes[1].deleted, true);
199+
});
200+
201+
test('a normalized false is an explicit override, distinct from "no opinion"', async () => {
202+
globalThis.databases = makeFakeDatabases({ all: { scope: 'all', paused: true } });
203+
204+
// paused:false must win over a cluster pause...
205+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-a', paused: false }), {
206+
paused: false,
207+
source: 'node',
208+
});
209+
// ...whereas undefined would silently fall through to it. This is exactly the confusion
210+
// threading `intent.paused` avoids.
211+
assert.deepEqual(await getDesiredPause('node-a', { scope: 'node-a', paused: undefined }), {
212+
paused: true,
213+
source: 'cluster',
214+
});
215+
});

0 commit comments

Comments
 (0)