Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 96 additions & 2 deletions services/sandbox/src/backend/kubernetes/k8s-backend.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
staleLifetimeCutoffMs,
} from './k8s-backend.ts';
import { podNameFor } from './k8s-pod-spec.ts';
import { formatResultLine } from './k8s-protocol.ts';

const cfg: SpawnerConfig = {
backend: 'kubernetes',
Expand Down Expand Up @@ -227,7 +228,7 @@ function stubClient(behavior: {
createSecret?: () => Promise<unknown>;
createPod?: () => Promise<unknown>;
readPod?: () => Promise<V1Pod>;
readLog?: () => Promise<string>;
readLog?: (p: { container: string }) => Promise<string>;
}): { core: CoreV1Api; namespace: string; calls: StubCalls } {
const calls: StubCalls = { deletes: [], replaces: 0 };
// oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- test stub
Expand All @@ -238,7 +239,8 @@ function stubClient(behavior: {
readNamespacedPod:
behavior.readPod ??
(() => Promise.reject(Object.assign(new Error('404'), { code: 404 }))),
readNamespacedPodLog: behavior.readLog ?? (() => Promise.resolve('')),
readNamespacedPodLog: (p: { container: string }) =>
behavior.readLog?.(p) ?? Promise.resolve(''),
replaceNamespacedSecret: () => {
calls.replaces += 1;
return Promise.resolve({});
Expand Down Expand Up @@ -334,3 +336,95 @@ describe('duplicate-executionId safety', () => {
);
});
});

// ---- stdout kubelet log rotation ------------------------------------------

describe('stdout log-rotation pinning', () => {
test('kubelet log rotation: canonical stdout is the pre-rotation head, not the new-file window', async () => {
// Runner emits PRE then kubelet rotates — the new file only contains
// ROTATED (shorter). Without the fix the final readPodLog returns ROTATED
// (a mid-stream window). With the fix the spawner-side accumulation
// retains PRE_HEAD (the deterministic head of the original log).
const PRE_HEAD = 'pre-rotation-head\n'; // 19 chars
const ROTATED = 'r\n'; // 2 chars — shorter, triggers shrink detection

const harvestLog = formatResultLine({
exitCode: 0,
stderr: '',
stderrTruncated: false,
outputFiles: [],
truncatedFiles: 0,
uploadStats: { attempted: 0, succeeded: 0, failures: [] },
quotaExhausted: false,
uploadFailed: false,
reportFailed: false,
readFailed: false,
stageMs: 0,
harvestMs: 0,
uploadMs: 0,
});

let runnerLogIdx = 0;
const runnerLogs = [PRE_HEAD, ROTATED];

let podCallIdx = 0;

// oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- test stub
const client = {
namespace: 'tale-sandbox',
core: {
createNamespacedSecret: () => Promise.resolve({}),
createNamespacedPod: () => Promise.resolve({}),
readNamespacedPod: () => {
podCallIdx += 1;
if (podCallIdx === 1) {
// waitForRunnerStart: runner container is Running
return Promise.resolve({
metadata: {},
status: {
containerStatuses: [
{
name: 'runner',
state: { running: { startedAt: new Date() } },
},
],
},
} as V1Pod);
}
// Main loop: pod Succeeded with harvest terminated
return Promise.resolve({
metadata: {},
status: {
phase: 'Succeeded',
containerStatuses: [
{ name: 'harvest', state: { terminated: { exitCode: 0 } } },
],
},
} as V1Pod);
Comment on lines +372 to +403

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine the flagged lines
if [ -f "services/sandbox/src/backend/kubernetes/k8s-backend.test.ts" ]; then
  echo "=== File exists, checking line count ==="
  wc -l services/sandbox/src/backend/kubernetes/k8s-backend.test.ts
  echo ""
  echo "=== Lines 360-410 (context around 372-403) ==="
  sed -n '360,410p' services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | cat -n
else
  echo "File not found at specified path"
fi

Repository: tale-project/tale

Length of output: 2253


🏁 Script executed:

# Search for stubClient helper in the codebase
echo "=== Searching for stubClient function ==="
rg "stubClient" --type ts --type tsx

Repository: tale-project/tale

Length of output: 149


🏁 Script executed:

# Search for other test fixtures in the same test file
echo "=== Searching for fixture patterns in k8s-backend.test.ts ==="
rg "const.*=.*\{" services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | head -20

Repository: tale-project/tale

Length of output: 884


🏁 Script executed:

# Look for stubClient implementation and usage in the test file
echo "=== Searching for stubClient usage context ==="
rg -A 10 "const stub = stubClient" services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | head -50

Repository: tale-project/tale

Length of output: 1529


🏁 Script executed:

# Check line 418 for secondary issue
echo "=== Lines 410-425 to check line 418 ==="
sed -n '410,425p' services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | cat -n

Repository: tale-project/tale

Length of output: 890


🏁 Script executed:

# Find the stubClient definition/import
echo "=== Looking for stubClient import or definition ==="
head -50 services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | cat -n

Repository: tale-project/tale

Length of output: 2159


🏁 Script executed:

# Search for stubClient definition
echo "=== Looking for stubClient definition ==="
rg "function stubClient|const stubClient|export.*stubClient" services/sandbox/src/backend/kubernetes/ -A 15

Repository: tale-project/tale

Length of output: 1837


🏁 Script executed:

# Also check if it's imported or defined in the test file
echo "=== Full search for stubClient in k8s-backend.test.ts ==="
rg -n "stubClient" services/sandbox/src/backend/kubernetes/k8s-backend.test.ts

Repository: tale-project/tale

Length of output: 280


🏁 Script executed:

# Look for pod() and secret() helper definitions
echo "=== Looking for pod() and secret() helper definitions ==="
rg -B 5 "function pod|const pod.*=" services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | head -40

Repository: tale-project/tale

Length of output: 234


🏁 Script executed:

# Get the full stubClient definition
echo "=== Full stubClient definition ==="
sed -n '227,266p' services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | cat -n

Repository: tale-project/tale

Length of output: 1962


🏁 Script executed:

# Get the pod() helper definition
echo "=== pod() helper definition ==="
rg -A 20 "^function pod\(" services/sandbox/src/backend/kubernetes/k8s-backend.test.ts | head -40

Repository: tale-project/tale

Length of output: 722


Remove type assertions from the test client fixture.

The inlined client object at lines 372-403 uses as V1Pod (twice) and as unknown as CoreV1Api assertions, which violate the coding guideline: **/*.{ts,tsx} — "Never as, never any, never unknown".

Instead of inlining the client, use the existing stubClient() helper with typed fixtures. This pattern is already used throughout the file and keeps test stubs assertion-free:

Refactor to stubClient with typed fixtures
-    // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- test stub
-    const client = {
-      namespace: 'tale-sandbox',
-      core: {
-        createNamespacedSecret: () => Promise.resolve({}),
-        createNamespacedPod: () => Promise.resolve({}),
-        readNamespacedPod: () => {
-          podCallIdx += 1;
-          if (podCallIdx === 1) {
-            return Promise.resolve({
-              metadata: {},
-              status: {
-                containerStatuses: [
-                  {
-                    name: 'runner',
-                    state: { running: { startedAt: new Date() } },
-                  },
-                ],
-              },
-            } as V1Pod);
-          }
-          return Promise.resolve({
-            metadata: {},
-            status: {
-              phase: 'Succeeded',
-              containerStatuses: [
-                { name: 'harvest', state: { terminated: { exitCode: 0 } } },
-              ],
-            },
-          } as V1Pod);
-        },
-        readNamespacedPodLog: ({ container }: { container: string }) => {
-          if (container === 'harvest') return Promise.resolve(harvestLog);
-          const log =
-            runnerLogs[runnerLogIdx] ?? runnerLogs[runnerLogs.length - 1];
-          runnerLogIdx += 1;
-          return Promise.resolve(log ?? '');
-        },
-        replaceNamespacedSecret: () => Promise.resolve({}),
-        deleteNamespacedPod: () => Promise.resolve({}),
-        deleteNamespacedSecret: () => Promise.resolve({}),
-        listNamespacedPod: () => Promise.resolve({ items: [] }),
-        listNamespacedSecret: () => Promise.resolve({ items: [] }),
-      } as unknown as CoreV1Api,
-    };
+    const startedPod: V1Pod = {
+      metadata: {},
+      status: {
+        containerStatuses: [
+          { name: 'runner', state: { running: { startedAt: new Date() } } },
+        ],
+      },
+    };
+    const succeededPod: V1Pod = {
+      metadata: {},
+      status: {
+        phase: 'Succeeded',
+        containerStatuses: [
+          { name: 'harvest', state: { terminated: { exitCode: 0 } } },
+        ],
+      },
+    };
+
+    const { core, namespace } = stubClient({
+      readPod: () => {
+        podCallIdx += 1;
+        return Promise.resolve(podCallIdx === 1 ? startedPod : succeededPod);
+      },
+      readLog: ({ container }) => {
+        if (container === 'harvest') return Promise.resolve(harvestLog);
+        const log =
+          runnerLogs[runnerLogIdx] ?? runnerLogs[runnerLogs.length - 1];
+        runnerLogIdx += 1;
+        return Promise.resolve(log ?? '');
+      },
+    });
+    const client = { namespace, core };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/sandbox/src/backend/kubernetes/k8s-backend.test.ts` around lines 372
- 403, Remove the inlined client object fixture that contains type assertions
(the `as V1Pod` and `as unknown as CoreV1Api` assertions) and refactor it to use
the existing `stubClient()` helper function with properly typed fixtures
instead. Look for other examples of `stubClient()` usage elsewhere in the test
file to follow the same pattern. This approach will eliminate the unsafe type
assertions while maintaining the same test behavior and keeping all test stubs
assertion-free according to the project's guidelines.

Source: Coding guidelines

},
readNamespacedPodLog: ({ container }: { container: string }) => {
if (container === 'harvest') return Promise.resolve(harvestLog);
// Runner: serve logs in sequence; cap at last entry
const log =
runnerLogs[runnerLogIdx] ?? runnerLogs[runnerLogs.length - 1];
runnerLogIdx += 1;
return Promise.resolve(log ?? '');
},
replaceNamespacedSecret: () => Promise.resolve({}),
deleteNamespacedPod: () => Promise.resolve({}),
deleteNamespacedSecret: () => Promise.resolve({}),
listNamespacedPod: () => Promise.resolve({ items: [] }),
listNamespacedSecret: () => Promise.resolve({ items: [] }),
} as unknown as CoreV1Api,
};

const backend = new KubernetesBackend(cfg, client);
const res = await backend.execute(cfg, req, execOpts());

expect(res.status).toBe('completed');
// The canonical stdout must be the pre-rotation head, not the rotated window.
expect(Buffer.from(res.stdoutBase64, 'base64').toString()).toBe('pre-rotation-head\n');
// The rotation must be flagged as truncation.
expect(res.truncated.stdout).toBe(true);
});
});
35 changes: 21 additions & 14 deletions services/sandbox/src/backend/kubernetes/k8s-backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,12 @@ export class KubernetesBackend implements ExecutionBackend {
let lastLogLen = 0;
let logShrunk = false;
let loggedPollError = false;
// Spawner-side canonical stdout buffer: accumulated from polled deltas
// so that a kubelet log rotation never gives us a mid-stream window.
// After rotation logs.length resets to 0; a final readPodLog would then
// return only the new file's content — the head of the original stream
// is gone. By accumulating here we always retain the deterministic head.
let logBuf = '';
const pollRunnerStdout = async (): Promise<void> => {
let logs: string;
try {
Expand All @@ -446,12 +452,18 @@ export class KubernetesBackend implements ExecutionBackend {
return;
}
if (logs.length > lastLogLen) {
scanner.onStdoutChunk?.(Buffer.from(logs.slice(lastLogLen), 'utf8'));
const delta = logs.slice(lastLogLen);
scanner.onStdoutChunk?.(Buffer.from(delta, 'utf8'));
lastLogLen = logs.length;
if (Buffer.byteLength(logBuf, 'utf8') < cfg.stdoutMaxBytes) {
({ text: logBuf } = capText(logBuf + delta, cfg.stdoutMaxBytes));
}
} else if (logs.length < lastLogLen) {
// The kubelet rotated the container log out from under us — the
// canonical head is gone, so the final read is a partial window.
// Kubelet rotated the container log out from under us — the
// canonical head is already captured in logBuf. Reset the length
// cursor so we continue accumulating from the new file's beginning.
logShrunk = true;
lastLogLen = 0;
}
};

Expand Down Expand Up @@ -540,19 +552,14 @@ export class KubernetesBackend implements ExecutionBackend {
await sleep(POLL_INTERVAL_MS, opts.signal);
}

// Final stdout read (the runner may have emitted more between the last
// poll and exit) → feed the residual to the scanner, then drain it. The
// canonical buffer is the full (capped) runner log.
// Final stdout poll (the runner may have emitted more between the last
// loop iteration and exit) → feed the residual to the scanner, then
// drain it. Use the spawner-side accumulation (logBuf) as the canonical
// stdout — it always holds the deterministic head of the stream even
// across kubelet log rotations.
await pollRunnerStdout();
scanner.finalize();
let stdout = '';
try {
stdout = await readPodLog(this.client, podName, 'runner', {
limitBytes: cfg.stdoutMaxBytes,
});
} catch (err) {
console.warn('[sandbox.k8s] final runner log read failed:', err);
}
const stdout = logBuf;
Comment on lines +555 to +562

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle final-iteration log shrink with one extra read before finalize.

If the final poll only observes logs.length < lastLogLen, the cursor is reset but no follow-up read happens before scanner.finalize(). That can miss immediately available post-rotation bytes when logBuf is still below cap.

💡 Suggested patch
       await pollRunnerStdout();
+      if (
+        logShrunk &&
+        Buffer.byteLength(logBuf, 'utf8') < cfg.stdoutMaxBytes
+      ) {
+        // If shrink is first detected in the final poll, do one extra read so
+        // we can capture the new file head before finalizing.
+        await pollRunnerStdout();
+      }
       scanner.finalize();
       const stdout = logBuf;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/sandbox/src/backend/kubernetes/k8s-backend.ts` around lines 555 -
562, In the final stdout handling block around pollRunnerStdout and
scanner.finalize, add a check after the final poll to detect if the logs have
shrunk (logs.length < lastLogLen indicating a potential rotation). When a shrink
is detected and logBuf is still below capacity, perform one additional poll or
read operation to capture any newly available post-rotation bytes before calling
scanner.finalize(). This ensures that immediately available bytes after log
rotation are not missed when feeding residual data to the scanner.

const stdoutStreamTruncated =
Buffer.byteLength(stdout, 'utf8') >= cfg.stdoutMaxBytes || logShrunk;

Expand Down