feat: add local worker activation and manifest supervision#70
Conversation
Sunrisea
changed the title
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Adds local worker management CLI (connect/list/status/disconnect/delete), worker manifest supervision with restart policies, safer plugin-probe package replacement (staging + backup), OTLP resource-attribute passthrough, and Claude Code transcript resume/synthetic-record fixes. Well-structured, good test coverage (10 test files), backward-compatible type changes, and proper sensitive-field filtering. Overall: solid contribution with minor suggestions below.
Findings
- [Warning]
src/flushers/otlp-trace-flusher.ts:448—agentConvertStatesMap keyed byagentType|resourceAttrscan grow unbounded if resource attributes vary; no eviction during normal operation. - [Warning]
src/deployment/plugin-probe-strategy.ts:400—fs.renameinrenamePathfails on cross-filesystem (EXDEV) scenarios; staging and dest may be on different mounts. - [Info]
src/local-workers/local-worker-activation-service.ts:44— SeparatePluginProbeStrategyinstance fromDeploymentManager's; both manage workers independently. - [Info]
src/local-workers/instance-store.ts:415— PID-file liveness check has inherent PID-reuse race. - [Info]
src/deployment/worker-manifest-supervisor.ts:417— Process-group signaling relies ondetached: true; platform assumption worth documenting.
Suggestions
- For the
agentConvertStatesMap: consider an LRU cap or periodic sweep of entries whose last-use timestamp exceeds a threshold. - For
renamePath: wrapfs.renamein a try/catch and fall back tofs.cp(source, target, {recursive: true})+fs.rm(source, {recursive: true, force: true})onEXDEV. - The transcript-parser changes for Claude Code resume/synthetic records are a good fix — the
fallbackTs/laterTimestampchain ensures all LLM calls get a validrequest_start_time.
Cross-repo Note
The resourceAttributes field added to AgentActivityEntry (src/types/events.ts) and the resourceAttributeKeys config option flow through to the OTLP flusher. If alibaba/loongsuite-python emits similar resource attributes, ensure the attribute key naming is consistent across both collectors.
Automated review by github-manager-bot
| this.dataDir = options.dataDir; | ||
| this.pilotDir = options.pilotDir; | ||
| this.definitions = options.definitions; | ||
| this.strategy = new PluginProbeStrategy(options.dataDir, options.pilotDir); |
There was a problem hiding this comment.
[Info] A separate PluginProbeStrategy instance is created here, independent of the one owned by DeploymentManager. Both hold their own WorkerManifestSupervisor. If both try to start/stop the same worker concurrently, there could be a race. Consider sharing the strategy instance or documenting the coordination contract.
|
|
||
| function isAlive(pid: number): boolean { | ||
| try { | ||
| process.kill(pid, 0); |
There was a problem hiding this comment.
[Info] PID-file-based liveness check via process.kill(pid, 0) has an inherent PID-reuse race: a recycled PID could produce a false-positive "running" result. This is a well-known limitation of PID file management and acceptable here, but worth noting for future hardening (e.g., writing a start-time stamp and verifying it).
|
|
||
| private signalProcessGroup(pgid: number, signal: NodeJS.Signals): boolean { | ||
| try { | ||
| process.kill(-pgid, signal); |
There was a problem hiding this comment.
[Info] process.kill(-pgid, signal) signals the entire process group. This relies on detached: true (line 170) making the child its own session leader, which works on Linux/macOS. Worth adding a brief comment documenting this platform assumption for future maintainers.
linrunqi08
left a comment
linrunqi08
left a comment
There was a problem hiding this comment.
Code Review: 独立性分析与对已有功能的影响
整体评价
PR 引入了 local worker 管理功能，整体设计较合理。新增的核心模块（instance-store.ts、worker-cli.ts、local-worker-activation-service.ts、worker-manifest-supervisor.ts）之间职责划分清晰，测试覆盖也比较充分。
独立性分析
新增模块（独立性好）：
src/local-workers/目录下的 3 个文件是全新模块，互相依赖但与已有代码耦合度较低src/deployment/worker-manifest-supervisor.ts作为新增的进程管理模块，仅被PluginProbeStrategy引用assets/hooks/shared/resource-context.mjs是独立的工具模块
对已有功能的影响点：
plugin-probe-strategy.ts— deploy 流程变更：deploy()从原来的"原地解压"改为"staging + backup + rename"，这影响所有 plugin-probe 类型的部署（不仅限于 worker）。好处是更安全（原子替换），但引入了新的失败模式transcript-parser.mjs— 时间戳隔离改为 per-promptId：lastToolResultTs从全局变量改为 per-promptId Map，修正了跨 turn 时间戳污染的问题。但这会改变已有多 turn + tool 场景下的request_start_time数值，可能影响下游 dashboardhook-processor.mjs— offset 更新时机：把_next_transcript_offset的更新移到turns.length === 0检查之前，改变了空 turn 场景的行为otlp-trace-flusher.ts— convertState 键值变更：从按agentType分组改为按agentType + resourceAttributes分组，对无 worker 的现有部署无影响（resourceAttributes 为空时 key 等价），但引入了潜在的内存增长问题
发现的问题
以下是具体的 inline comments，按严重程度排序。
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review after new commits. Adds local worker instance management (connect/list/status/disconnect/delete) and a worker.manifest.json supervisor with restart policies — solid, well-tested feature. Bootstrap tokens are stored with 0o600 permissions, worker processes are spawned (not exec'd) and managed as detached process groups, and shutdown is wired into the orchestrator lifecycle. Overall the implementation is clean; the findings below are minor hardening suggestions, none blocking.
Suggestions
- Add
.unref()to the activation-service poll interval so an abnormal exit path cannot leave the process dangling. - Consider an
exit-event race instead ofkill(pid,0)polling inwaitForExit. - Switch test
execSynctar calls toexecFileSyncto avoid shell interpolation.
Cross-repo Note
No API/protocol surface shared with loongsuite-python; the local-worker model is pilot-internal.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [INFO]
tests/unit/deployment/plugin-probe-strategy.test.ts:423— [Info]execSyncwith string-interpolated paths is a shell-injection smell, even in tests. PreferexecFileSync('tar', ['-czf', tarball, '-C', destDir, '.'], { stdio: 'ignore' })to bypass the shell entirely. Low priority, test-only. (line outside diff)
| } | ||
|
|
||
| const intervalMs = Number(process.env.LOONGSUITE_LOCAL_WORKER_SCAN_INTERVAL_MS) || DEFAULT_SCAN_INTERVAL_MS; | ||
| this.timer = setInterval(() => void this.refresh('poll'), intervalMs); |
There was a problem hiding this comment.
[Warning] This setInterval keeps the Node.js event loop alive if stop() is not reached (e.g. an unhandled early-exit path). worker-manifest-supervisor.ts already calls .unref() on its restart timer (line 353) — consider mirroring that here: this.timer = setInterval(...); this.timer.unref(); for consistency and safer lifecycle.
| } | ||
| } | ||
|
|
||
| private async waitForExit(pid: number, timeoutMs: number): Promise<void> { |
There was a problem hiding this comment.
[Info] waitForExit busy-polls with process.kill(pid, 0) every 100ms. Functionally correct for the graceful-shutdown path, but consider racing the child's exit event against a timeout Promise instead — avoids repeated signal-zero probes and is slightly more efficient. Non-blocking.
fangxiu-wf
left a comment
fangxiu-wf
left a comment
There was a problem hiding this comment.
已读 ralf0131 + linrunqi08 的 review,绝大部分 worker 相关问题不在我熟悉范围内。
我专注 review 了 claude-code 相关 4 个文件(claude-code-hook-processor.mjs / transcript-parser.mjs / shared/resource-context.mjs / 间接相关的 otlp-trace-flusher.ts),结合已有评论补充几条独立观察。
不点 APPROVE 不是因为 claude-code 部分有问题 — 它通过我所有测试和质量评估;留给熟悉 worker 模块的 reviewer 拍板整体合并。
一个 PR 流程性建议:当前 PR base 在 e6ac2b1,早于 #67 合并到 main。我本地 simulate merge main 后 typecheck ✓,关键测试 43/43 通过(buildTurnRecords 同时承载 intercept 和 resourceAttributes 两组逻辑,语义不冲突),所以不会 merge 失败,但 reviewer 看到的 diff 跟实际合并后状态有差距,建议 author rebase / merge main 后再 force-push 让 review 更直观。
| if (!ts) return; | ||
| const key = promptMapKey(promptId); | ||
| const prev = lastToolResultTsByPromptId.get(key); | ||
| if (!prev || ts > prev) lastToolResultTsByPromptId.set(key, ts); |
There was a problem hiding this comment.
[Praise — 独立认领] 这处 fix(lastToolResultTs 按 promptId 分桶 + 下游 splitIntoTurns 第 396-401 行的 per-llmCall fallback chain)正好解决了我们最近发现的客户问题 — 多 turn 长会话中,中间某条 user record 缺 timestamp,导致后续 llm.request 的 time_unix_nano="0",被下游 util-genai 误转成 Unix epoch (1970) 起点的怪异 LLM span(duration ≈ 56 年)。我们前几天还在内部讨论修法。
你们这版方案比我们当时设计的"扩展兜底链"更根本:
- 消除了跨 promptId 污染(用 Map 隔离)
normalizeRequestStart多了 candidate > response 反向时序的防御
可选建议:PR description 加一句这个 motivation,后续维护者(包括我们自己)会感谢。
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| const MAX_RESOURCE_FIELD_VALUE_LENGTH = 512; | ||
| const SENSITIVE_FIELD_NAME_RE = /(?:TOKEN|SECRET|PASSWORD|KEY|CREDENTIAL|HEADER|COOKIE)/i; |
There was a problem hiding this comment.
[Nit — DRY + 精度,non-blocking] SENSITIVE_FIELD_NAME_RE 和 src/flushers/otlp-trace-flusher.ts 里的 SENSITIVE_RESOURCE_KEY_RE 是字字相同的正则,两处独立维护;建议提取到一个共享常量(或从本文件 export 出来给 flusher 复用)。
另外正则本身是子串匹配,可能误杀合规字段:LOOKUP_KEY / HEADER_NAME / MONKEY_PATCH 这类 metadata 会因含 KEY / HEADER 等被拒。当前 DEFAULT_RESOURCE_ENV_FIELD_MAP 只配了 WORKER_NAME / INSTANCE_ID,实际触发面为零;但未来扩展或用户自定义 fieldMap 会踩。
建议改成精确模式,如:
/(^|_)(TOKEN|SECRET|PASSWORD|CREDENTIAL|COOKIE)(_|$)|^(API_KEY|API_HEADER)$/i当前用法无害,可作为后续小改进。
| const traceId = generateTraceId(); | ||
| const entrySpanId = generateSpanId(); | ||
| const agentSpanId = generateSpanId(); | ||
| const collectedResourceAttributes = collectResourceAttributesFromEnv(process.env, { agentId: AGENT_ID }); |
There was a problem hiding this comment.
[Minor optimization — optional] collectResourceAttributesFromEnv(process.env, ...) 在 buildTurnRecords 每次调用都重扫一遍 env。env 在 hook 进程生命周期内不会变(hook process 是 per-Stop spawn 的一次性进程),建议提到模块顶层算一次:
const RESOURCE_ATTRIBUTES = collectResourceAttributesFromEnv(process.env, { agentId: AGENT_ID });一个 session 通常调几次 buildTurnRecords,实际收益微乎其微,但代码更干净。
| return state; | ||
| } | ||
|
|
||
| private evictConvertStates(): void { |
There was a problem hiding this comment.
[Edge case 附议] 附议 @ralf0131 和 @linrunqi08 关于 agentConvertStates 无界增长的关切,MAX_CONVERT_STATES + LRU 的设计方向正确。
一个未在他们评论里覆盖的边界:evictConvertStates 通过 [...entries].find(([, state]) => state.active === 0) 找可踢的;如果 64 个 state 全部 active>0(理论可能,例如同 batch 内对 64 个不同 worker 并发 convert),find 返回 undefined → 不踢 → 新进的 state 让 agentConvertStates.size 临时超过 64。
不致命(active 会很快下降,下个 evict 会清理),但严格说违反 cap。这是有意为之(优先正确性而非严格 cap)还是希望 await 等闲下来?当前在常规负载下无问题,只是确认设计意图,可能值得在 method 注释里说明。
| return bMs > aMs ? b : a; | ||
| } | ||
|
|
||
| function normalizeRequestStart(candidate, responseTs) { |
There was a problem hiding this comment.
[Doc nit] normalizeRequestStart 里 candidate > response 时返回 responseTs 的逻辑挺巧妙(防御 transcript 异常顺序导致 request_start 比 response 还晚的逻辑矛盾),但函数名只说 "normalize",不直接看代码不容易理解为什么这么做。建议加一行 inline 注释说明 motivation:
// Defense against transcript anomalies: a request_start_time later than
// its own response timestamp is logically impossible. Cap the candidate
// so the span doesn't get a negative duration in OTLP.
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review after the two follow-up commits (fix: address local worker review nits, fix: address claude trace review nits). The previous findings are addressed well:
setIntervalnow calls.unref()— the timer no longer keeps the event loop alive (Warning fixed).- Resource attributes are collected once at module level instead of per-turn — avoids redundant parsing on every turn (perf improvement).
- The
agentConvertStateseviction path is documented with its correctness rationale (temporary overflow preferred over dropping an active provider).
CLA is signed. Solid follow-up — thank you for iterating on the feedback.
Findings
- [Info]
otlp-trace-flusher.ts/resource-context.mjs— The sensitive-field regex was tightened from a loose substring match (TOKEN|SECRET|PASSWORD|KEY|CREDENTIAL|HEADER|COOKIE) to delimited-token matching plus exactAPI_KEY/API_HEADER. This correctly reduces false positives (e.g. a field likeKEYBOARD), but note thatKEYandHEADERare no longer matched as substrings at all — so a field such ashttp.header.authorizationorprivate.keywould no longer be redacted. If any mapped env var could surface such names, consider addingauthorizationor a delimitedHEADERtoken back; otherwise the tradeoff is acceptable.
Notes
- CI status is currently
UNSTABLE— only thelicense/clacheck is visible. As a first-time contributor, GitHub Actions workflows may require maintainer approval to run. Once CI is green this looks good to go.
Automated review by github-manager-bot
4 participants
Summary
This PR adds local remote-managed worker support to loongsuite-pilot and hardens the plugin-probe runtime package lifecycle.
Main changes:
loongsuite-pilot workercommands for local worker instance management:connectliststatusdisconnectdeleteworker.manifest.jsonsupervision for plugin-probe packages:Validation
Validated locally:
npm run typechecktests/unit/deployment/plugin-probe-strategy.test.tstests/unit/local-workers/local-worker-activation-service.test.tstests/unit/local-workers/instance-store.test.tstests/unit/local-workers/worker-cli.test.tstests/unit/hooks/shared/resource-context.test.mjstests/unit/flushers/otlp-trace-flusher/conversion.test.tstests/unit/hooks/claude-code/hook-processor.test.mjstests/unit/hooks/claude-code/transcript-parser.test.mjstests/unit/inputs/base/hook-record-transform.test.tsValidated on a remote Linux host using the open-source branch plus existing server-side old runtime bundle and
agents.d:feat/remote-managed-local-workers-sync4928735npm cinpm run buildnpm run typecheckagents.drunningworker listshowedworker-01--plugin-install-scope local--model-config-mode managed-runtimeNotes
This PR intentionally does not include internal AgentTeams runtime templates or bundled runtime tarballs. Runtime templates and bundles can be provided by deployment packaging or local installation separately.