fix(jaeger,du): codex review, service-scoped reads and a real request deadline
read() served a trace fetched by id through any service directory, so
/services/<other>/traces/<id>.json returned content that stat and ls both
report absent. It now asserts the service exists and that the trace's own
process table names it. Membership comes from the trace document, not the
service listing, because that listing is windowed and limited and would hide a
trace that really does belong.
The typescript jaeger transport ignored requestTimeout: the config field, the
snake-case mapping and the transport option all existed, but nothing read them
and fetch ran with no deadline, so a stalled endpoint hung the command forever.
Threaded through as seconds, matching python's httpx timeout and dify's
existing requestTimeout convention.
du's operand stat caught every exception and reported it as
"No such file or directory", so an auth failure or a backend bug printed a
wrong reason and a partial total. Narrowed to isMissingPath, matching python's
(FileNotFoundError, ValueError). The test mock rejected with a bare
Error('ENOENT') rather than a stamped FsError, which is what let this pass.
Moved the jaeger resource's command and op imports to module scope, per the
repo's import rule. Verified no cycle: the registry and workspace still import
cleanly. This also matches the typescript resource, which already imports
JAEGER_COMMANDS at module scope.
Integ gains a cat and a stat through a foreign service, the pair that proves
the two agree.
This commit is contained in:
@@ -246,6 +246,32 @@
|
||||
"stdout": "",
|
||||
"stderr": "stat: /j/services/checkout-api/traces/00000000000000000000000000000000.json: No such file or directory\n"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "jg_foreign_service_cat",
|
||||
"seq": 610019,
|
||||
"targets": [
|
||||
"jaeger"
|
||||
],
|
||||
"command": "cat /j/services/search-api/traces/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1.json",
|
||||
"expect": {
|
||||
"exit": 1,
|
||||
"stdout": "",
|
||||
"stderr": "cat: /j/services/search-api/traces/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1.json: No such file or directory\n"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "jg_foreign_service_stat",
|
||||
"seq": 610020,
|
||||
"targets": [
|
||||
"jaeger"
|
||||
],
|
||||
"command": "stat /j/services/search-api/traces/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1.json",
|
||||
"expect": {
|
||||
"exit": 1,
|
||||
"stdout": "",
|
||||
"stderr": "stat: /j/services/search-api/traces/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1.json: No such file or directory\n"
|
||||
}
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -29,6 +29,29 @@ def _json_bytes(data: Any) -> bytes:
|
||||
return json.dumps(data, ensure_ascii=False, indent=2).encode()
|
||||
|
||||
|
||||
def _has_service(trace: dict[str, Any], service: str) -> bool:
|
||||
"""Report whether any span in the trace was emitted by the service.
|
||||
|
||||
A trace is fetched by id from the global endpoint, so the id alone does not
|
||||
place it under the service directory it was addressed through. Membership
|
||||
is read from the trace's own process table rather than the service listing,
|
||||
which is windowed and limited and would hide a trace that really belongs.
|
||||
|
||||
Args:
|
||||
trace (dict[str, Any]): trace document from the API.
|
||||
service (str): service name the path addressed.
|
||||
|
||||
Returns:
|
||||
bool: True when the service emitted at least one span.
|
||||
"""
|
||||
processes = trace.get("processes")
|
||||
if not isinstance(processes, dict):
|
||||
return False
|
||||
return any(
|
||||
isinstance(p, dict) and p.get("serviceName") == service
|
||||
for p in processes.values())
|
||||
|
||||
|
||||
async def read(
|
||||
accessor: JaegerAccessor,
|
||||
path: PathSpec,
|
||||
@@ -62,17 +85,23 @@ async def read(
|
||||
return _json_bytes(operations)
|
||||
|
||||
if scope.level == "trace":
|
||||
assert scope.service is not None
|
||||
assert scope.trace_id is not None
|
||||
# A malformed id cannot name an existing trace, so it is ENOENT rather
|
||||
# than the API's 400 "invalid length for TraceID".
|
||||
if not is_trace_id(scope.trace_id):
|
||||
raise enoent(virtual)
|
||||
await assert_service(accessor, scope.service, virtual)
|
||||
try:
|
||||
trace = await fetch_trace(accessor, scope.trace_id)
|
||||
except JaegerApiError as exc:
|
||||
if exc.status_code == 404:
|
||||
raise enoent(virtual) from exc
|
||||
raise
|
||||
# Reading by id would otherwise serve any trace through any service
|
||||
# directory, contradicting stat and ls for the same path.
|
||||
if not _has_service(trace, scope.service):
|
||||
raise enoent(virtual)
|
||||
return _json_bytes(trace)
|
||||
|
||||
raise enoent(virtual)
|
||||
|
||||
@@ -15,7 +15,9 @@
|
||||
from typing import Any
|
||||
|
||||
from mirage.accessor.jaeger import JaegerAccessor
|
||||
from mirage.commands.builtin.jaeger import COMMANDS
|
||||
from mirage.core.jaeger.readdir import readdir
|
||||
from mirage.ops.jaeger import OPS as JAEGER_VFS_OPS
|
||||
from mirage.resource.base import BaseResource
|
||||
from mirage.resource.jaeger.config import JaegerConfig
|
||||
from mirage.resource.jaeger.prompt import PROMPT
|
||||
@@ -36,9 +38,6 @@ class JaegerResource(BaseResource):
|
||||
super().__init__()
|
||||
self.config = config
|
||||
self.accessor = JaegerAccessor(self.config)
|
||||
from mirage.commands.builtin.jaeger import COMMANDS
|
||||
from mirage.ops.jaeger import OPS as JAEGER_VFS_OPS
|
||||
|
||||
for command in COMMANDS:
|
||||
self.register(command)
|
||||
for op in JAEGER_VFS_OPS:
|
||||
|
||||
@@ -50,7 +50,18 @@ def known_service():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_trace(accessor, index):
|
||||
doc = {"traceID": TRACE_A, "spans": [{"operationName": "POST /checkout"}]}
|
||||
doc = {
|
||||
"traceID": TRACE_A,
|
||||
"spans": [{
|
||||
"operationName": "POST /checkout",
|
||||
"processID": "p1"
|
||||
}],
|
||||
"processes": {
|
||||
"p1": {
|
||||
"serviceName": "checkout"
|
||||
}
|
||||
},
|
||||
}
|
||||
with known_service():
|
||||
with patch("mirage.core.jaeger.read.fetch_trace",
|
||||
new_callable=AsyncMock,
|
||||
@@ -61,6 +72,46 @@ async def test_read_trace(accessor, index):
|
||||
assert json.loads(raw) == doc
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_trace_rejects_foreign_service(accessor, index):
|
||||
# stat and ls report this path absent, so cat must agree; reading by id
|
||||
# would otherwise serve any trace through any service directory.
|
||||
doc = {
|
||||
"traceID": TRACE_A,
|
||||
"spans": [{
|
||||
"operationName": "POST /checkout",
|
||||
"processID": "p1"
|
||||
}],
|
||||
"processes": {
|
||||
"p1": {
|
||||
"serviceName": "checkout"
|
||||
}
|
||||
},
|
||||
}
|
||||
with patch("mirage.core.jaeger.readdir.fetch_services",
|
||||
new_callable=AsyncMock,
|
||||
return_value=["checkout", "search"]):
|
||||
with patch("mirage.core.jaeger.read.fetch_trace",
|
||||
new_callable=AsyncMock,
|
||||
return_value=doc):
|
||||
with pytest.raises(FileNotFoundError):
|
||||
await read(accessor,
|
||||
spec(f"services/search/traces/{TRACE_A}.json"),
|
||||
index)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_trace_rejects_unknown_service(accessor, index):
|
||||
with known_service():
|
||||
with patch("mirage.core.jaeger.read.fetch_trace",
|
||||
new_callable=AsyncMock,
|
||||
return_value={"traceID": TRACE_A}) as fetch:
|
||||
with pytest.raises(FileNotFoundError):
|
||||
await read(accessor,
|
||||
spec(f"services/nope/traces/{TRACE_A}.json"), index)
|
||||
fetch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_operations(accessor, index):
|
||||
ops = [{"name": "POST /checkout", "spanKind": "server"}]
|
||||
|
||||
@@ -16,6 +16,7 @@ import { DU_BUILDER } from './du.ts'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { materialize } from '../../../../io/types.ts'
|
||||
import { FileStat, FileType, PathSpec } from '../../../../types.ts'
|
||||
import { enoent } from '../../../../utils/errors.ts'
|
||||
import type { Accessor } from '../../../../accessor/base.ts'
|
||||
import type { CommandIO } from '../adapter.ts'
|
||||
|
||||
@@ -40,7 +41,9 @@ const OPS: CommandIO = {
|
||||
readStream: () => emptyStream(),
|
||||
stat: (_a, p) => {
|
||||
const node = TREE[p.virtual]
|
||||
if (node === undefined) return Promise.reject(new Error('ENOENT'))
|
||||
// A stamped FsError, as every real backend raises: the builder tells a
|
||||
// missing operand from a backend failure by the code, not the message.
|
||||
if (node === undefined) return Promise.reject(enoent(p.virtual))
|
||||
return Promise.resolve(
|
||||
new FileStat({
|
||||
name: p.virtual,
|
||||
@@ -115,6 +118,22 @@ describe('du walk fallback (no native du op)', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('a backend failure propagates instead of reading as a missing operand', async () => {
|
||||
const failing: CommandIO = {
|
||||
...OPS,
|
||||
stat: () => Promise.reject(new Error('403 Forbidden')),
|
||||
}
|
||||
await expect(
|
||||
DU_BUILDER.fn(failing, ACCESSOR, [PathSpec.fromStrPath('/db')], [], {
|
||||
stdin: null,
|
||||
flags: {},
|
||||
filetypeFns: null,
|
||||
cwd: '/',
|
||||
resource: {} as never,
|
||||
}),
|
||||
).rejects.toThrow('403 Forbidden')
|
||||
})
|
||||
|
||||
it('-h renders human-readable sizes', async () => {
|
||||
expect(await runDu([PathSpec.fromStrPath('/db')], { h: true })).toEqual(['5B\t/db'])
|
||||
})
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
// limitations under the License.
|
||||
// ========= Copyright 2026 @ Strukto.AI All Rights Reserved. =========
|
||||
|
||||
import { isMissingPath } from '../../../../utils/errors.ts'
|
||||
import { rekey } from '../../../../utils/key_prefix.ts'
|
||||
import type { Accessor } from '../../../../accessor/base.ts'
|
||||
import type { IndexCacheStore } from '../../../../cache/index/store.ts'
|
||||
@@ -67,7 +68,11 @@ export const DU_BUILDER: Builder = {
|
||||
try {
|
||||
await ops.stat(accessor, p, idx)
|
||||
present.push(p)
|
||||
} catch {
|
||||
} catch (err) {
|
||||
// Only a genuinely absent path is an operand error. An auth failure,
|
||||
// a transport error or a backend bug must not read back as "missing",
|
||||
// which would print a wrong reason and a partial total.
|
||||
if (!isMissingPath(err)) throw err
|
||||
errors.push(`du: cannot access '${p.rawPath}': No such file or directory`)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -13,7 +13,27 @@
|
||||
// ========= Copyright 2026 @ Strukto.AI All Rights Reserved. =========
|
||||
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { JaegerApiError, fetchTraces, isTraceId, type JaegerTransport } from './_client.ts'
|
||||
import {
|
||||
HttpJaegerTransport,
|
||||
JaegerApiError,
|
||||
fetchTraces,
|
||||
isTraceId,
|
||||
type JaegerTransport,
|
||||
} from './_client.ts'
|
||||
|
||||
// Captures the fetch init so the request deadline can be asserted, and never
|
||||
// settles on its own so only the abort can end the request.
|
||||
class StalledTransport extends HttpJaegerTransport {
|
||||
init: RequestInit | undefined
|
||||
protected override readonly fetch: typeof fetch = (_url, init) => {
|
||||
this.init = init
|
||||
return new Promise((_resolve, reject) => {
|
||||
init?.signal?.addEventListener('abort', () => {
|
||||
reject(new Error('aborted by deadline'))
|
||||
})
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
class RecordingTransport implements JaegerTransport {
|
||||
readonly calls: { path: string; query?: Record<string, string | number | undefined> }[] = []
|
||||
@@ -73,3 +93,11 @@ describe('jaeger fetchTraces window', () => {
|
||||
await expect(fetchTraces(transport, 'checkout')).rejects.toBeInstanceOf(JaegerApiError)
|
||||
})
|
||||
})
|
||||
|
||||
describe('jaeger request deadline', () => {
|
||||
it('aborts a stalled request after the configured timeout', async () => {
|
||||
const transport = new StalledTransport({ timeout: 0.01 })
|
||||
await expect(transport.request('/api/services')).rejects.toThrow('aborted by deadline')
|
||||
expect(transport.init?.signal).toBeInstanceOf(AbortSignal)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -44,9 +44,13 @@ export interface JaegerTransport {
|
||||
|
||||
export interface HttpJaegerTransportOptions {
|
||||
host?: string
|
||||
// Seconds, mirroring python's JaegerConfig.request_timeout and the
|
||||
// requestTimeout config field it is normalized from.
|
||||
timeout?: number
|
||||
}
|
||||
|
||||
const DEFAULT_TIMEOUT_SECONDS = 30
|
||||
|
||||
function buildUrl(
|
||||
base: string,
|
||||
path: string,
|
||||
@@ -78,18 +82,23 @@ function errorMessage(body: unknown, status: number): string {
|
||||
export class HttpJaegerTransport implements JaegerTransport {
|
||||
protected readonly fetch: typeof fetch = globalThis.fetch.bind(globalThis)
|
||||
private readonly host: string
|
||||
private readonly timeoutSeconds: number
|
||||
|
||||
constructor(opts: HttpJaegerTransportOptions = {}) {
|
||||
this.host = opts.host ?? 'http://localhost:16686'
|
||||
this.timeoutSeconds = opts.timeout ?? DEFAULT_TIMEOUT_SECONDS
|
||||
}
|
||||
|
||||
async request(
|
||||
path: string,
|
||||
query: Record<string, string | number | undefined> = {},
|
||||
): Promise<unknown> {
|
||||
// Without a deadline a stalled Jaeger endpoint hangs the command forever;
|
||||
// python gets this from the httpx timeout.
|
||||
const res = await this.fetch(buildUrl(this.host, path, query), {
|
||||
method: 'GET',
|
||||
headers: { Accept: 'application/json' },
|
||||
signal: AbortSignal.timeout(this.timeoutSeconds * 1000),
|
||||
})
|
||||
let body: unknown
|
||||
try {
|
||||
|
||||
@@ -64,7 +64,11 @@ class ThrowingTransport implements JaegerTransport {
|
||||
|
||||
describe('jaeger read', () => {
|
||||
it('renders a trace document', async () => {
|
||||
const doc = { traceID: TRACE_A, spans: [{ operationName: 'POST /checkout' }] }
|
||||
const doc = {
|
||||
traceID: TRACE_A,
|
||||
spans: [{ operationName: 'POST /checkout', processID: 'p1' }],
|
||||
processes: { p1: { serviceName: 'checkout' } },
|
||||
}
|
||||
const transport = new RecordingTransport({
|
||||
...SERVICES,
|
||||
[`/api/traces/${TRACE_A}`]: { data: [doc] },
|
||||
@@ -77,6 +81,41 @@ describe('jaeger read', () => {
|
||||
expect(JSON.parse(DEC.decode(bytes))).toEqual(doc)
|
||||
})
|
||||
|
||||
it('refuses a trace addressed through a service that did not emit it', async () => {
|
||||
// stat and ls report this path absent, so cat must agree; reading by id
|
||||
// would otherwise serve any trace through any service directory.
|
||||
const doc = {
|
||||
traceID: TRACE_A,
|
||||
spans: [{ operationName: 'POST /checkout', processID: 'p1' }],
|
||||
processes: { p1: { serviceName: 'checkout' } },
|
||||
}
|
||||
const transport = new RecordingTransport({
|
||||
'/api/services': { data: ['checkout', 'search'] },
|
||||
[`/api/traces/${TRACE_A}`]: { data: [doc] },
|
||||
})
|
||||
await expect(
|
||||
read(
|
||||
accessor(transport),
|
||||
spec(`/services/search/traces/${TRACE_A}.json`),
|
||||
new RAMIndexCacheStore(),
|
||||
),
|
||||
).rejects.toMatchObject({ code: 'ENOENT' })
|
||||
})
|
||||
|
||||
it('refuses a trace under an unknown service', async () => {
|
||||
const transport = new RecordingTransport({
|
||||
...SERVICES,
|
||||
[`/api/traces/${TRACE_A}`]: { data: [{ traceID: TRACE_A }] },
|
||||
})
|
||||
await expect(
|
||||
read(
|
||||
accessor(transport),
|
||||
spec(`/services/nope/traces/${TRACE_A}.json`),
|
||||
new RAMIndexCacheStore(),
|
||||
),
|
||||
).rejects.toMatchObject({ code: 'ENOENT' })
|
||||
})
|
||||
|
||||
it('renders the operations list', async () => {
|
||||
const ops = [{ name: 'POST /checkout', spanKind: 'server' }]
|
||||
const transport = new RecordingTransport({ ...SERVICES, '/api/operations': { data: ops } })
|
||||
|
||||
@@ -26,6 +26,23 @@ function toJsonBytes(data: unknown): Uint8Array {
|
||||
return ENC.encode(JSON.stringify(data, null, 2))
|
||||
}
|
||||
|
||||
// Whether any span in the trace was emitted by the service. A trace is fetched
|
||||
// by id from the global endpoint, so the id alone does not place it under the
|
||||
// service directory it was addressed through. Membership is read from the
|
||||
// trace's own process table rather than the service listing, which is windowed
|
||||
// and limited and would hide a trace that really belongs.
|
||||
function hasService(trace: unknown, service: string): boolean {
|
||||
if (trace === null || typeof trace !== 'object') return false
|
||||
const processes = (trace as { processes?: unknown }).processes
|
||||
if (processes === null || typeof processes !== 'object') return false
|
||||
return Object.values(processes as Record<string, unknown>).some(
|
||||
(p) =>
|
||||
p !== null &&
|
||||
typeof p === 'object' &&
|
||||
(p as { serviceName?: unknown }).serviceName === service,
|
||||
)
|
||||
}
|
||||
|
||||
export async function read(
|
||||
accessor: JaegerAccessor,
|
||||
path: PathSpec,
|
||||
@@ -44,17 +61,23 @@ export async function read(
|
||||
}
|
||||
|
||||
if (scope.level === 'trace') {
|
||||
const service = scope.service ?? ''
|
||||
const traceId = scope.traceId ?? ''
|
||||
// A malformed id cannot name an existing trace, so it is ENOENT rather
|
||||
// than the API's 400 "invalid length for TraceID".
|
||||
if (!isTraceId(traceId)) throw enoent(path)
|
||||
await assertService(accessor, service, path)
|
||||
let trace: unknown
|
||||
try {
|
||||
const trace = await fetchTrace(accessor.transport, traceId)
|
||||
return toJsonBytes(trace)
|
||||
trace = await fetchTrace(accessor.transport, traceId)
|
||||
} catch (err) {
|
||||
if (err instanceof JaegerApiError && err.status === 404) throw enoent(path)
|
||||
throw err
|
||||
}
|
||||
// Reading by id would otherwise serve any trace through any service
|
||||
// directory, contradicting stat and ls for the same path.
|
||||
if (!hasService(trace, service)) throw enoent(path)
|
||||
return toJsonBytes(trace)
|
||||
}
|
||||
|
||||
throw enoent(path)
|
||||
|
||||
@@ -52,8 +52,9 @@ export class JaegerResource extends BaseResource implements Resource {
|
||||
constructor(config: JaegerConfig) {
|
||||
super()
|
||||
this.config = config
|
||||
const transportOpts: { host?: string } = {}
|
||||
const transportOpts: { host?: string; timeout?: number } = {}
|
||||
if (config.host !== undefined) transportOpts.host = config.host
|
||||
if (config.requestTimeout !== undefined) transportOpts.timeout = config.requestTimeout
|
||||
const accessorConfig: {
|
||||
defaultTraceLimit?: number
|
||||
defaultFromTimestamp?: string
|
||||
|
||||
Reference in New Issue
Block a user