Skip to content

Commit 0f5bed4

Browse files
fix(pipeline): baseline double-count, counter drift, retest totals, loop stop
From the bug hunt, adversarially reviewed (safe to commit, no CRITICAL/HIGH): - baselineHistory double-count: refreshBaseline() ran per node ATTEMPT (before testNode), so a stop+resume or pause-retry of one node pushed multiple baseline samples (user saw baseline=4 vs speed=3 for 3 nodes). Now refreshBaseline only stashes a pending sample; commitBaselineSample(addr) pushes once per RECORDED node (dedup by address, per-invocation closure), at the success/fail result sites — interrupted/retried attempts commit nothing. baseline == tested-count. - runAudit now calls recomputeCounters(state) at completion (only on finish/stop, not the error path) so persisted counters match results[] instead of the drift-prone live increments. - Iron-Rule retest block now guards the success flip on result.actualMbps!=null (mirrors the internet-recovery block); a null-mbps result persists as a fail without a bogus failed--/tested++. - runRetestSkips no longer rewrites totalNodes = tested+failed (which collapsed the grand total for partial/stopped runs) — uses Math.max(results.length,total). - fresh runAudit now resets skippedNodes alongside the sibling counters. - continuous pause()/stop() now propagate to the in-flight pipeline (triggerPipelineStop() + loopState.stopRequested) so a sweep and its payments halt promptly instead of running to completion first. Test suite green (188/31/45/35).
1 parent aa2d951 commit 0f5bed4

2 files changed

Lines changed: 58 additions & 4 deletions

File tree

audit/continuous.js

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import { EventEmitter } from 'events';
2323
import { writeFileSync, readFileSync, existsSync, mkdirSync } from 'fs';
2424
import path from 'path';
2525
import { fileURLToPath } from 'url';
26-
import { createState, setActiveDbRunId, getActiveDbRunId } from './pipeline.js';
26+
import { createState, setActiveDbRunId, getActiveDbRunId, triggerPipelineStop } from './pipeline.js';
2727
import { sleep } from '../protocol/speedtest.js';
2828

2929
const __dirname = path.dirname(fileURLToPath(import.meta.url));
@@ -144,6 +144,10 @@ const _ctrl = {
144144
paused: false,
145145
pausedBatch: null, // { batchId, mode, planId, subscriptionId, subscriptionGranter, frozenNodes, testedAddrs }
146146
resumeIntent: null, // { batchId, frozenNodes, testedAddrs } — set by resume(), consumed by _runLoop
147+
// The pipeline state object for the currently in-flight pass. Held so pause()/stop()
148+
// can raise its stopRequested flag and abort the running runAudit/runSubPlanTest sweep
149+
// promptly, instead of letting the whole pass (and its payments) finish first.
150+
loopState: null,
147151
};
148152

149153
// Injected pipeline runner (overridable in tests)
@@ -371,6 +375,8 @@ async function _runLoop() {
371375
// without this, every continuous-loop result fell back to sdk:'js'.
372376
const loopState = createState();
373377
loopState.stopRequested = false;
378+
// Expose the in-flight state so pause()/stop() can abort the running pipeline.
379+
_ctrl.loopState = loopState;
374380
if (_ctrl.activeSDK) loopState.activeSDK = _ctrl.activeSDK;
375381
if (_ctrl.pricingMode) loopState.pricingMode = _ctrl.pricingMode;
376382
loopState.runMode = _ctrl.mode || 'p2p';
@@ -613,6 +619,7 @@ async function _runLoop() {
613619
} finally {
614620
_ctrl.running = false;
615621
_ctrl.stopRequested = false;
622+
_ctrl.loopState = null; // no in-flight pass anymore
616623
_persistLoopConfig();
617624
_emitScoped('loop:stopped', {
618625
iterations: _ctrl.iteration,
@@ -728,6 +735,10 @@ export async function start(opts = {}) {
728735
export function stop() {
729736
if (!_ctrl.running) return { ok: true, alreadyStopped: true };
730737
_ctrl.stopRequested = true;
738+
// Propagate to the in-flight pipeline so the running runAudit/runSubPlanTest sweep
739+
// (and its payments) returns promptly instead of completing the whole pass first.
740+
if (_ctrl.loopState) _ctrl.loopState.stopRequested = true;
741+
triggerPipelineStop();
731742
_emitScoped('loop:stopping', {});
732743
return { ok: true };
733744
}
@@ -744,6 +755,11 @@ export function pause() {
744755
if (_ctrl.paused) return { ok: false, error: 'already paused' };
745756
// Raise stopRequested so the in-flight pipeline returns.
746757
_ctrl.stopRequested = true;
758+
// Propagate to the in-flight pipeline state + wake its stop-aware sleeps so the
759+
// running sweep aborts promptly, rather than finishing the full pass (and its
760+
// payments) before the pause takes effect — matching this function's docstring.
761+
if (_ctrl.loopState) _ctrl.loopState.stopRequested = true;
762+
triggerPipelineStop();
747763
// Sentinel for _runLoop: break out without clearing state.
748764
_ctrl.paused = true;
749765
_emitScoped('loop:stopping', {});

audit/pipeline.js

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,7 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
617617
results.length = 0;
618618
state.testedNodes = 0;
619619
state.failedNodes = 0;
620+
state.skippedNodes = 0;
620621
state.passed15 = 0;
621622
state.passed10 = 0;
622623
state.passedBaseline = 0;
@@ -666,15 +667,32 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
666667
// ── On-chain reporter (opt-in) ─────────────────────────────────────────
667668
_initOnchainReporter(account, client, state, broadcast);
668669

670+
// Latest baseline sample measured for the node currently in-flight. refreshBaseline()
671+
// (called once per ATTEMPT) only refreshes the live state.baselineMbps and stashes the
672+
// sample here; commitBaselineSample() pushes it into baselineHistory at most once per
673+
// node that actually records a result. This keeps the HISTORY append aligned with
674+
// processed nodes (1 per result) instead of per attempt — an interrupted/pause-retried
675+
// node no longer double-appends and drifts baselineHistory above nodeSpeedHistory.
676+
let _pendingBaselineSample = null;
677+
let _lastBaselineHistoryAddr = null;
669678
async function refreshBaseline() {
670679
try {
671680
const bl = await speedtestDirect();
672681
state.baselineMbps = bl.mbps;
673682
const ts = new Date().toLocaleTimeString('en-US', { hour12: false });
674-
state.baselineHistory = [...state.baselineHistory, { mbps: bl.mbps, ts }].slice(-10);
683+
_pendingBaselineSample = { mbps: bl.mbps, ts };
675684
broadcast('state', { state });
676685
} catch { }
677686
}
687+
// Append the pending baseline sample for `addr` at most once. Deduping by address means
688+
// a node re-tested after Stop/Resume or a pause-retry (j--) appends exactly one sample.
689+
function commitBaselineSample(addr) {
690+
if (!_pendingBaselineSample) return;
691+
if (addr && addr === _lastBaselineHistoryAddr) return;
692+
state.baselineHistory = [...state.baselineHistory, _pendingBaselineSample].slice(-10);
693+
_lastBaselineHistoryAddr = addr || null;
694+
_pendingBaselineSample = null;
695+
}
678696

679697
// ── Phase 1: Fetch node list ───────────────────────────────────────────
680698
let allNodes;
@@ -909,6 +927,7 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
909927
} else {
910928
state.failedNodes++;
911929
}
930+
commitBaselineSample(node.address); // 1 baseline sample per recorded node
912931
upsertResult(result); // success — no snippet needed
913932
state.resumeHeadAddr = null;
914933
saveResults();
@@ -977,6 +996,7 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
977996
// FAIL result — zero-skip: explicit failure
978997
const failResult = buildFailResult(node, status, state, errMsg, error?.diag || {});
979998
state.failedNodes++;
999+
commitBaselineSample(node.address); // 1 baseline sample per recorded node
9801000
upsertResult(failResult, _logSnippet);
9811001
state.resumeHeadAddr = null;
9821002
saveResults();
@@ -1169,14 +1189,23 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
11691189
break;
11701190
}
11711191

1172-
if (result) {
1192+
if (result && result.actualMbps != null) {
1193+
// Only a real-speed result flips this node from failed → tested.
11731194
state.failedNodes = Math.max(0, state.failedNodes - 1);
11741195
state.testedNodes++;
11751196
if (result.pass10mbps) state.passed10++;
11761197
upsertResult(result);
11771198
saveResults();
11781199
broadcast('result', { result, state });
11791200
broadcast('log', { msg: ` ✓ Retest PASS: ${result.actualMbps} Mbps` });
1201+
} else if (result) {
1202+
// Truthy result but null mbps (e.g. SESSION_UNMAPPED) — still a failure (it
1203+
// was already counted as failed before this retest). Persist without touching
1204+
// counters; flipping to "tested" here would wrongly clear a still-failed node.
1205+
upsertResult(result, _sanitizeSnippet(_ironSnippet));
1206+
saveResults();
1207+
broadcast('result', { result, state });
1208+
broadcast('log', { msg: ` ✗ Retest FAIL: ${result.errorCode || 'no speed'}` });
11801209
} else {
11811210
const errMsg = error?.message || 'Unknown';
11821211
const failResult = buildFailResult(node, status, state, errMsg, error?.diag || {});
@@ -1195,6 +1224,10 @@ export async function runAudit(resume, state, broadcast, preloadedNodes = null,
11951224

11961225
const finalCache = getCacheStats();
11971226
await _finalizeOnchainReporter();
1227+
// Reconcile the live-incremented counters against results[] so the final
1228+
// persisted summary matches the authoritative classifier (the per-node
1229+
// increments above can drift across Stop/Resume, pause-retries and retests).
1230+
recomputeCounters(state);
11981231
state.status = 'done';
11991232
state.completedAt = new Date().toISOString();
12001233
state.currentNode = null;
@@ -1218,7 +1251,12 @@ export async function runRetestSkips(skipAddrs, state, broadcast) {
12181251
state.retestPassed = 0;
12191252
state.retestFailed = 0;
12201253
recomputeCounters(state);
1221-
state.totalNodes = state.testedNodes + state.failedNodes; // show grand total
1254+
// Preserve the run's true node count. Deriving totalNodes from tested+failed
1255+
// collapses the grand total for a run that was Stopped partway (skipped /
1256+
// TEST_RUN_SKIP rows are classified as skippedNodes, not failed, so they'd
1257+
// vanish from the total and break "remaining"). results.length is the real
1258+
// count of rows; keep whichever is larger so an already-correct chain total wins.
1259+
state.totalNodes = Math.max(results.length, state.totalNodes || 0); // show grand total
12221260
clearPoisonedSessions();
12231261
clearPaidNodes();
12241262
invalidateSessionCache(); // Force fresh session lookups — prevents stale mappings

0 commit comments

Comments
 (0)