fix(plugins): treat an HTTP error response as a failed delivery
Found by driving the core against a real local server rather than an injected delivery stub, which is exactly where it could hide: fetch only rejects on a network-level failure, so a 500, a 503 or a 429 resolved normally and the batch was counted as delivered and dropped. The unit tests could not catch it because their stub throws, and real fetch does not. That is the likelier outage than a refused connection, so the retry added in the previous commit was covering the rarer half of the problem. Any non-2xx now throws and takes the retry path. Matching the Python core, which retries every HTTP error rather than classifying them: the backoff and the queue bound contain a payload that will never be accepted, because the re-queued batch sits at the front and is the first thing evicted. Two tests against the real default delivery path, stubbing fetch rather than the delivery hook, so a 503 keeps the batch and a 200 clears it. End to end against a local server, real fetch and real retry timing: events arrive and carry a uuid, a 503 keeps the batch, an immediate retry is suppressed by the backoff, the retained event is delivered on recovery with its original uuid and no duplicate, and a refused connection behaves the same way. Nine of nine. Identity checked on the same path: opencode's project_hash is salted, differs per account, is stable within one, and no raw project id appears in the payload. openclaw emits two distinct identities across a key change, which is the defect this branch fixes, observed on the wire rather than through a mock. agent-plugin-core/ts 32, openclaw 431, opencode 35, pi-agent 89, deepseek 47, Python core 242 unaffected. Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
This commit is contained in:
@@ -94,12 +94,19 @@ export function createTelemetry(config: TelemetryConfig) {
|
||||
const maxQueueSize = config.maxQueueSize ?? 100;
|
||||
|
||||
const deliver = config.delivery ?? (async (batch: Record<string, unknown>[]) => {
|
||||
await fetch(POSTHOG_BATCH_URL, {
|
||||
const response = await fetch(POSTHOG_BATCH_URL, {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({ api_key: POSTHOG_API_KEY, batch }),
|
||||
signal: AbortSignal.timeout(3_000),
|
||||
});
|
||||
// fetch only rejects on a network-level failure. Without this check a 500,
|
||||
// a 503 or a 429 resolved normally and the batch was counted as delivered
|
||||
// and dropped, which is the likelier outage than a refused connection.
|
||||
// Any non-2xx is retried, matching the Python core: the backoff and the
|
||||
// queue bound contain a payload that will never be accepted, because the
|
||||
// re-queued batch sits at the front and is the first thing evicted.
|
||||
if (!response.ok) throw new Error(`posthog responded ${response.status}`);
|
||||
});
|
||||
|
||||
async function flush(): Promise<void> {
|
||||
|
||||
@@ -214,3 +214,46 @@ test("a long outage costs the oldest events, not unbounded memory", async () =>
|
||||
assert.ok(telemetry.queueForTesting().length <= 3, "queue grew past maxQueueSize");
|
||||
telemetry.resetForTesting();
|
||||
});
|
||||
|
||||
test("an HTTP error response is a failure, not a delivery", async () => {
|
||||
// fetch only rejects on a network-level failure, so a 500 used to resolve
|
||||
// normally and the batch was dropped as delivered. Exercises the real default
|
||||
// delivery path rather than an injected one, which is where this hid.
|
||||
const realFetch = globalThis.fetch;
|
||||
let calls = 0;
|
||||
globalThis.fetch = (async () => {
|
||||
calls += 1;
|
||||
return new Response("upstream is unwell", { status: 503 });
|
||||
}) as typeof fetch;
|
||||
|
||||
const telemetry = createTelemetry({
|
||||
host: "h", source: "S", version: "1", distinctId: "d", flushThreshold: 1000,
|
||||
});
|
||||
try {
|
||||
telemetry.capture("during.outage");
|
||||
await telemetry.flush();
|
||||
|
||||
assert.equal(calls, 1, "never reached the network");
|
||||
assert.equal(telemetry.queueForTesting().length, 1, "a 503 was counted as delivered");
|
||||
} finally {
|
||||
globalThis.fetch = realFetch;
|
||||
telemetry.resetForTesting();
|
||||
}
|
||||
});
|
||||
|
||||
test("a 2xx is a delivery", async () => {
|
||||
const realFetch = globalThis.fetch;
|
||||
globalThis.fetch = (async () => new Response("ok", { status: 200 })) as typeof fetch;
|
||||
|
||||
const telemetry = createTelemetry({
|
||||
host: "h", source: "S", version: "1", distinctId: "d", flushThreshold: 1000,
|
||||
});
|
||||
try {
|
||||
telemetry.capture("fine");
|
||||
await telemetry.flush();
|
||||
assert.equal(telemetry.queueForTesting().length, 0, "a good response did not clear the queue");
|
||||
} finally {
|
||||
globalThis.fetch = realFetch;
|
||||
telemetry.resetForTesting();
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user