Merge branch 'main' of https://github.com/sharon77242/Argus

Author: sharon77242

packages/agent/src/analysis/fs-analyzer.ts

@@ -50,7 +50,7 @@ export class FsAnalyzer {
       if (entry) {
         if (now - entry.firstSeen <= this.READ_WINDOW_MS) {
           entry.count++;
-          if (entry.count === this.READ_THRESHOLD) {
+          if (entry.count >= this.READ_THRESHOLD) {
             suggestions.push({
               severity: "warning",
               rule: "missing-fs-cache",
@@ -67,8 +67,12 @@ export class FsAnalyzer {
         this.recentReads.set(filePath, { count: 1, firstSeen: now });
       }
 
-      // Cleanup
-      if (this.recentReads.size > 100) this.recentReads.clear();
+      // Evict only stale entries — never wipe the whole map
+      if (this.recentReads.size > 100) {
+        for (const [k, v] of this.recentReads) {
+          if (now - v.firstSeen > this.READ_WINDOW_MS) this.recentReads.delete(k);
+        }
+      }
     }
 
     return suggestions;

packages/agent/src/analysis/log-analyzer.ts

@@ -46,7 +46,7 @@ export class LogAnalyzer {
       const now = Date.now();
       if (now - this.recentErrors.firstSeen <= this.ERROR_WINDOW_MS) {
         this.recentErrors.count++;
-        if (this.recentErrors.count === this.ERROR_THRESHOLD) {
+        if (this.recentErrors.count >= this.ERROR_THRESHOLD) {
           suggestions.push({
             severity: "critical",
             rule: "log-error-storm",

packages/agent/src/analysis/query-analyzer.ts

@@ -204,8 +204,13 @@ export class QueryAnalyzer {
   private detectNPlusOne(query: string, suggestions: FixSuggestion[]): void {
     const now = Date.now();
 
-    // Normalize the query to detect repeated structural patterns
-    const normalized = query.replace(/\?\s*/g, "?").trim();
+    // Normalize all placeholder styles to '?' so structurally identical queries
+    // from different drivers (pg $N, named :param, mysql ?) collapse to the same key.
+    const normalized = query
+      .replace(/\$\d+/g, "?")
+      .replace(/:[a-zA-Z_]\w*/g, "?")
+      .replace(/\?\s*/g, "?")
+      .trim();
 
     const entry = this.recentQueries.get(normalized);
 

packages/agent/src/export/exporter.ts

@@ -49,7 +49,7 @@ export class OTLPExporter {
 
     for (let attempt = 0; attempt <= maxRetries; attempt++) {
       if (attempt > 0) {
-        await new Promise((r) => setTimeout(r, retryDelayMs * attempt));
+        await new Promise((r) => setTimeout(r, retryDelayMs * 2 ** (attempt - 1)));
       }
       try {
         await this.attempt(events);

packages/agent/src/instrumentation/http.ts

@@ -94,7 +94,7 @@ export class HttpInstrumentation extends EventEmitter {
         once: (event: string, cb: () => void) => void;
         statusCode?: number;
       };
-      r.once("close", () => {
+      r.once("end", () => {
         onEnd(r.statusCode);
       });
     });

packages/agent/tests/analysis/fs-analyzer.test.ts

@@ -43,4 +43,29 @@ describe("FsAnalyzer", () => {
     const suggestions = analyzer.analyze("readFile", "/var/log/app.log");
     assert.strictEqual(suggestions.length, 0);
   });
+
+  // Bug: threshold check used === so warning fired on exactly the 5th read and never again
+  it("should keep warning on reads beyond the threshold (>= not ===)", () => {
+    const fresh = new FsAnalyzer();
+    const path = "/data/keeps-firing.json";
+    for (let i = 0; i < 5; i++) fresh.analyze("readFile", path);
+    // 6th read must also produce the warning
+    const suggestions = fresh.analyze("readFile", path);
+    const rule = suggestions.find((s) => s.rule === "missing-fs-cache");
+    assert.ok(rule, "missing-fs-cache should still fire on the 6th read");
+  });
+
+  // Bug: when recentReads.size > 100 the entire map was cleared, losing all tracking state
+  it("should not lose tracking state when more than 100 unique paths are active", () => {
+    const fresh = new FsAnalyzer();
+    // Fill the map with 100 unique paths
+    for (let i = 0; i < 100; i++) fresh.analyze("readFile", `/unique/path/${i}.json`);
+    // Now analyse a path 4 times (below threshold) — state must survive the eviction check
+    const target = "/data/tracked.json";
+    for (let i = 0; i < 4; i++) fresh.analyze("readFile", target);
+    // 5th read triggers the threshold — must still fire despite size > 100
+    const suggestions = fresh.analyze("readFile", target);
+    const rule = suggestions.find((s) => s.rule === "missing-fs-cache");
+    assert.ok(rule, "tracking state must survive when map has >100 entries");
+  });
 });

packages/agent/tests/analysis/log-analyzer.test.ts

@@ -40,4 +40,15 @@ describe("LogAnalyzer", () => {
     assert.strictEqual(s1.length, 0);
     assert.strictEqual(s2.length, 0);
   });
+
+  // Bug: threshold check used === so warning fired only on exactly the 5th error and never again
+  it("should keep warning on every error beyond the threshold (>= not ===)", () => {
+    const fresh = new LogAnalyzer();
+    // Reach threshold
+    for (let i = 0; i < 5; i++) fresh.analyze(["err"], "error");
+    // 6th error must also trigger the storm warning
+    const suggestions = fresh.analyze(["err"], "error");
+    const rule = suggestions.find((s) => s.rule === "log-error-storm");
+    assert.ok(rule, "log-error-storm should still fire on the 6th error in the window");
+  });
 });

packages/agent/tests/analysis/query-analyzer.test.ts

@@ -79,4 +79,30 @@ describe("QueryAnalyzer", () => {
     const serious = suggestions.filter((s) => s.severity !== "info" && s.rule !== "n-plus-one");
     assert.strictEqual(serious.length, 0, "Well-formed query should have no serious issues");
   });
+
+  // Bug: normalization only collapsed '?' — PostgreSQL $N and :name params were not normalized,
+  // so structurally identical pg queries were treated as distinct and N+1 was never detected.
+  it("should detect N+1 for PostgreSQL $N placeholder style", () => {
+    const pgAnalyzer = new QueryAnalyzer();
+    // Each call uses a different parameter number but the same structural query
+    for (let i = 1; i <= 4; i++) {
+      const s = pgAnalyzer.analyze(`SELECT id, name FROM users WHERE id = $${i}`);
+      assert.ok(!s.find((x) => x.rule === "n-plus-one"), `Should not trigger at call ${i}`);
+    }
+    const suggestions = pgAnalyzer.analyze("SELECT id, name FROM users WHERE id = $5");
+    const rule = suggestions.find((s) => s.rule === "n-plus-one");
+    assert.ok(rule, "Should detect N+1 for PostgreSQL $N style params");
+  });
+
+  it("should detect N+1 for named :param placeholder style", () => {
+    const namedAnalyzer = new QueryAnalyzer();
+    for (let i = 1; i <= 4; i++) {
+      namedAnalyzer.analyze(`SELECT * FROM orders WHERE user_id = :userId${i} LIMIT 10`);
+    }
+    const suggestions = namedAnalyzer.analyze(
+      "SELECT * FROM orders WHERE user_id = :userId5 LIMIT 10",
+    );
+    const rule = suggestions.find((s) => s.rule === "n-plus-one");
+    assert.ok(rule, "Should detect N+1 for named :param style params");
+  });
 });

packages/agent/tests/export/exporter.test.ts

@@ -34,6 +34,56 @@ describe("OTLPExporter", () => {
     );
   });
 
+  // Bug: retry delay used retryDelayMs * attempt (linear) but JSDoc promised "doubles each attempt".
+  // With maxRetries=3 the 3rd retry diverges: linear gives 300ms, exponential gives 400ms.
+  it("should use exponential backoff — delay doubles each retry attempt", async () => {
+    const capturedDelays: number[] = [];
+    const origSetTimeout = globalThis.setTimeout;
+    // Capture delay values; fire immediately so the test doesn't actually wait
+    (globalThis as any).setTimeout = function (fn: () => void, ms?: number) {
+      if (typeof ms === "number" && ms >= 100) capturedDelays.push(ms);
+      return origSetTimeout(fn, 0);
+    };
+
+    const requestMock = mock.method(https, "request", (_opts: unknown, callback?: unknown) => {
+      const reqMock = new EventEmitter() as any;
+      reqMock.write = () => {};
+      reqMock.end = () => {};
+      reqMock.destroy = () => {};
+      const resMock = new EventEmitter() as any;
+      resMock.statusCode = 503; // always retryable
+      process.nextTick(() => {
+        if (typeof callback === "function") callback(resMock);
+        resMock.emit("end");
+      });
+      return reqMock;
+    });
+
+    const exporter = new OTLPExporter({
+      endpointUrl: "https://example.com/v1/traces",
+      key: "k",
+      cert: "c",
+      ca: "ca",
+      maxRetries: 3,
+      retryDelayMs: 100,
+    });
+
+    try {
+      await exporter.export([
+        { id: "1", metricName: "lag", value: 50, payload: { timestamp: Date.now() } },
+      ]);
+    } catch {
+      // all 4 attempts failed — expected
+    } finally {
+      (globalThis as any).setTimeout = origSetTimeout;
+      requestMock.mock.restore();
+    }
+
+    // attempt 1: 100*2^0=100  attempt 2: 100*2^1=200  attempt 3: 100*2^2=400
+    // (linear would give 100, 200, 300 — fails on the last value)
+    assert.deepStrictEqual(capturedDelays, [100, 200, 400], "delays must be exponential");
+  });
+
   it("should construct mTLS options using node:https request correctly", async () => {
     const mockRequest = mock.method(https, "request", (options: any, callback?: any) => {
       // Assert mTLS options are populated exactly

packages/agent/tests/instrumentation/http.test.ts

@@ -86,6 +86,45 @@ describe("HttpInstrumentation", () => {
     );
   });
 
+  // Bug: instrumentation listened for 'close' on the response instead of 'end'.
+  // On HTTP keep-alive connections the socket never closes promptly, so 'close' on
+  // IncomingMessage fires only when the connection eventually times out — meaning
+  // the "request" telemetry event could be delayed by seconds or never arrive.
+  // The fix is to listen for 'end', which fires as soon as the response body is consumed.
+  it("should emit the 'request' event before the socket closes (end semantics)", async () => {
+    const instrumentation = new HttpInstrumentation(() => undefined);
+    const events: TracedHttpRequest[] = [];
+    instrumentation.on("request", (r) => events.push(r));
+    instrumentation.enable();
+
+    // Server with keep-alive enabled (Node.js default) — it will NOT close the socket
+    // after the response, so IncomingMessage 'close' would only fire after the timeout.
+    const server = http.createServer((_req, res) => {
+      res.setHeader("Connection", "keep-alive");
+      res.writeHead(200);
+      res.end("hello");
+    });
+
+    await new Promise<void>((r) => server.listen(0, r));
+    const { port } = server.address() as { port: number };
+
+    const req = http.request(`http://localhost:${port}/`, (res) => {
+      res.on("data", () => {}); // consume so 'end' fires
+    });
+    req.end();
+
+    // Wait enough for 'end' to fire (few ms), but not for keep-alive timeout (5 000ms)
+    await new Promise((r) => setTimeout(r, 50));
+
+    instrumentation.disable();
+    // Force-close all connections so the port is released immediately
+    (server as any).closeAllConnections?.();
+    server.close();
+
+    assert.strictEqual(events.length, 1, "'request' event must fire before socket closes");
+    assert.strictEqual(events[0].statusCode, 200);
+  });
+
   it("should not include correlationId when request is outside runWithContext", async () => {
     const instrumentation = new HttpInstrumentation(() => "test.ts:1");
     const requests: TracedHttpRequest[] = [];