fix: unblock the server event loop (0.0.40) - #61
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite intermittently died with "database is locked" - four times today, including once on CI for an unrelated PR, and repeatedly while verifying the event-loop fix. It opens 38 of its own DatabaseSync connections to ctrl-pane.db while the server process holds the same file, and with no busy timeout SQLite returns SQLITE_BUSY immediately rather than waiting for the writer to finish. Every test-side connection now waits up to 15s for a lock, which is what a real client does. Five consecutive native-world runs pass where the same build previously failed roughly one run in three. This is a harness race, not a product defect, and no assertion changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Version, changelog and the local/1helm-channel-machine:0.0.40 pins, plus the offline release fallback pointed at 0.0.40 with fail-closed placeholder digests until this commit's artifacts exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesAsynchronous memory and agent flows
Runtime readiness and OCI preparation
0.0.40 release metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant RuntimeReadiness
participant OCI
Client->>Server: request runtime status or startup
Server->>RuntimeReadiness: refresh readiness
RuntimeReadiness->>OCI: run asynchronous probe
OCI-->>RuntimeReadiness: return runtime state
RuntimeReadiness-->>Server: return cached or refreshed state
Server-->>Client: return readiness or startup result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/server/channel-computers.ts (1)
1922-1926: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the Linux invocation once.
Line 1922 builds a full invocation only to read
invocation.commandintohelper. Line 1924 then builds the same invocation again insideociAsync. Extract a small helper that returns the resolved command, or reuseinvocationfor the spawn.🤖 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 `@src/server/channel-computers.ts` around lines 1922 - 1926, Update the Linux OCI setup around linuxOciInvocation and ociAsync to construct the invocation only once. Reuse the existing invocation’s resolved command and spawn configuration for the version check, or extract a small helper that returns the resolved command while preserving the current timeout and behavior for both version and readiness checks.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@site/server.mjs`:
- Around line 43-52: Replace the PENDING_DIGEST placeholders in RELEASE_FALLBACK
with valid SHA-256 digests specific to each listed artifact, ensuring
latestLinuxRelease() accepts the offline fallback. Add coverage for
latestRelease/latestLinuxRelease with SITE_RELEASE_FETCH_DISABLED=1 and verify
the 0.0.40 fallback is served successfully.
In `@src/server/bots.ts`:
- Line 1574: Move the awaited buildContext call into the existing try block in
the turn-handling flow, keeping it after activeTurn registration and before the
code that uses messages. Ensure any rejection from buildContext is handled by
the surrounding try/finally so active turn cleanup, finalizeAgentTurn, and
working-message removal still execute.
- Around line 2002-2007: Isolate the two await rememberForAgent calls in the
turn-completion flow from the outer failure handler so memory persistence errors
cannot mark an already-rendered successful turn as failed. Update the writes
around the initial episode persistence and the channel-awareness persistence
inside the agent.kind === "channel" branch to catch or otherwise contain their
rejections, while preserving the existing success bookkeeping and response
behavior.
In `@src/server/channel-computers.ts`:
- Around line 1834-1858: Update pendingRuntimeReadiness for the OCI/Windows path
so installationScopedRuntimeName() is not invoked when the installation identity
is unavailable or malformed. Guard both runtime_scoped_name-dependent fields,
shared_runtime and storage_authority, returning safe null or fallback values
while preserving normal values once the identity is ready; ensure
runtimeReadiness continues returning its request-safe snapshot without throwing
during early startup.
- Around line 2030-2038: Update the runtime readiness refresh flow around
runtimeReadinessPass and the forced-refresh callers so a forced refresh does not
return an already-running probe started before the state change. Chain the
forced probe after the existing in-flight pass completes, then cache and return
the post-change probe result while preserving deduplication for non-forced
callers.
- Around line 1746-1757: Close the concurrency race in the prepare flow around
the initial running guard and the awaited ociChannelImageExists() call. Update
the relevant prepare function so concurrent callers cannot both pass the
idle-state check and create separate preparation passes: either mark the state
running before the first await or re-check the running guard immediately after
it. Preserve the existing completed-image handling for callers that observe an
available OCI channel image.
In `@test/event-loop-unblocking.mjs`:
- Around line 10-18: Update sourceBetween to validate that both start and end
markers are present and that end occurs after start before slicing the source.
Assert or fail clearly for missing or reordered markers, while preserving the
existing slice behavior for valid markers.
---
Nitpick comments:
In `@src/server/channel-computers.ts`:
- Around line 1922-1926: Update the Linux OCI setup around linuxOciInvocation
and ociAsync to construct the invocation only once. Reuse the existing
invocation’s resolved command and spawn configuration for the version check, or
extract a small helper that returns the resolved command while preserving the
current timeout and behavior for both version and readiness checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4de456b3-204e-4533-8931-7671efd2a0f6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
CHANGELOG.mdREADME.mdpackage.jsonscripts/run-test-suite.mjssite/server.mjssrc/server/agents.tssrc/server/bots.tssrc/server/channel-computers.tssrc/server/db.tssrc/server/history.tssrc/server/improvements.tssrc/server/index.tssrc/server/memory.tssrc/server/setup.tstest/autonomy-platform.mjstest/channel-computers-backend-child.mjstest/channel-computers.mjstest/event-loop-unblocking.mjstest/files-latency.mjstest/native-world.mjstest/sweep-server-integration.mjs
Comment on lines
+43
to
+52
| const PENDING_DIGEST = "pending-release-digest"; | ||
| const RELEASE_FALLBACK_TAG = "v0.0.40"; | ||
| const RELEASE_FALLBACK = { | ||
| tag_name: RELEASE_FALLBACK_TAG, | ||
| draft: false, | ||
| prerelease: false, | ||
| assets: [ | ||
| ["1Helm-0.0.39-arm64.dmg", "de381468a61edc6b5c4d84525792be84f7575ba36777d35119f5878a4298fd0b"], | ||
| ["1Helm-0.0.39-mac-arm64.zip", "a35ba43a5136592977acfde4e7ba97d1399b4916ff0c90fd192e312a88414547"], | ||
| ["1Helm-0.0.39-linux-node.tgz", "ac54f11c153e89b8534417b4bfaa8aed22ceb5f0a4b10f164902483ae180b077"], | ||
| ["1Helm-0.0.40-arm64.dmg", PENDING_DIGEST], | ||
| ["1Helm-0.0.40-mac-arm64.zip", PENDING_DIGEST], | ||
| ["1Helm-0.0.40-linux-node.tgz", PENDING_DIGEST], |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the placeholder digests before release.
When the GitHub release fetch fails, latestRelease() returns RELEASE_FALLBACK. latestLinuxRelease() then rejects pending-release-digest because it does not match the required SHA-256 format. The offline fallback therefore still fails instead of serving the 0.0.40 release. Supply an artifact-specific digest for each asset and add a test with SITE_RELEASE_FETCH_DISABLED=1.
🤖 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 `@site/server.mjs` around lines 43 - 52, Replace the PENDING_DIGEST
placeholders in RELEASE_FALLBACK with valid SHA-256 digests specific to each
listed artifact, ensuring latestLinuxRelease() accepts the offline fallback. Add
coverage for latestRelease/latestLinuxRelease with SITE_RELEASE_FETCH_DISABLED=1
and verify the 0.0.40 fallback is served successfully.
| if (!preparedMessageId) broadcastToChannel(channelId, { type: "message", message: serializeMessage(msgId), parent: serializeMessage(threadRootId) }); | ||
|
|
||
| const messages = buildContext(bot, agent, channelId, triggerId, threadRootId, fresh, hostAuthorized, hiddenContext, requestUserId); | ||
| const messages = await buildContext(bot, agent, channelId, triggerId, threadRootId, fresh, hostAuthorized, hiddenContext, requestUserId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move the awaited buildContext call inside the try block.
Line 1574 runs after activeTurn is registered in activeTurns (line 1462) and before the try at line 1577. buildContext now awaits recallForAgent, which calls the memory bridge subprocess. If that call rejects or times out, the rejection escapes the try/finally. Then turns.delete(activeTurn) never runs, finalizeAgentTurn never runs, and the _Working…_ message stays in the channel. The turn row remains in running state.
🛠️ Proposed fix
- const messages = await buildContext(bot, agent, channelId, triggerId, threadRootId, fresh, hostAuthorized, hiddenContext, requestUserId);
- const tools = toolsFor(bot, agent, hostAuthorized, channelId, requestUserId);
- const actor = agent?.kind === "skipper" ? "skipper" : "agent";
- try {
+ const actor = agent?.kind === "skipper" ? "skipper" : "agent";
+ try {
+ const messages = await buildContext(bot, agent, channelId, triggerId, threadRootId, fresh, hostAuthorized, hiddenContext, requestUserId);
+ const tools = toolsFor(bot, agent, hostAuthorized, channelId, requestUserId);🤖 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 `@src/server/bots.ts` at line 1574, Move the awaited buildContext call into the
existing try block in the turn-handling flow, keeping it after activeTurn
registration and before the code that uses messages. Ensure any rejection from
buildContext is handled by the surrounding try/finally so active turn cleanup,
finalizeAgentTurn, and working-message removal still execute.
Comment on lines
+2002
to
2007
| await rememberForAgent(agent, episode, { source: `1helm:thread:${threadId}:message:${msgId}`, importance: 0.62, metadata: { kind: "session-outcome", channel_id: channelId, thread_id: threadId, message_id: msgId }, sessionId: `thread:${threadId}` }); | ||
| if (agent.kind === "channel") { | ||
| const skipper = q1("SELECT a.*, NULL channel_id FROM agents a WHERE a.kind='skipper' AND a.status<>'deleted' LIMIT 1"); | ||
| const channelName = String(q1("SELECT name FROM channels WHERE id=?", channelId)?.name || channelId); | ||
| if (skipper) rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`, | ||
| if (skipper) await rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`, | ||
| { source: `1helm:channel:${channelId}:thread:${threadId}`, importance: 0.55, metadata: { kind: "channel-awareness", channel_id: channelId, thread_id: threadId, agent_id: agent.id }, sessionId: `channel:${channelId}` }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate memory-write failures from the turn outcome.
Lines 2002 and 2006 await rememberForAgent after the final answer is already painted. If the memory bridge rejects, control jumps to the catch at line 2013. That block sets turnFailed = true, marks progress rows failed, appends the error to the answer body, and sets the thread status to failed. A memory persistence failure then presents a completed turn as a failed turn.
Catch these two writes locally, or run them after the success bookkeeping at line 2010.
🛠️ Proposed fix
- await rememberForAgent(agent, episode, { source: `1helm:thread:${threadId}:message:${msgId}`, importance: 0.62, metadata: { kind: "session-outcome", channel_id: channelId, thread_id: threadId, message_id: msgId }, sessionId: `thread:${threadId}` });
- if (agent.kind === "channel") {
- const skipper = q1("SELECT a.*, NULL channel_id FROM agents a WHERE a.kind='skipper' AND a.status<>'deleted' LIMIT 1");
- const channelName = String(q1("SELECT name FROM channels WHERE id=?", channelId)?.name || channelId);
- if (skipper) await rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`,
- { source: `1helm:channel:${channelId}:thread:${threadId}`, importance: 0.55, metadata: { kind: "channel-awareness", channel_id: channelId, thread_id: threadId, agent_id: agent.id }, sessionId: `channel:${channelId}` });
- }
+ try {
+ await rememberForAgent(agent, episode, { source: `1helm:thread:${threadId}:message:${msgId}`, importance: 0.62, metadata: { kind: "session-outcome", channel_id: channelId, thread_id: threadId, message_id: msgId }, sessionId: `thread:${threadId}` });
+ if (agent.kind === "channel") {
+ const skipper = q1("SELECT a.*, NULL channel_id FROM agents a WHERE a.kind='skipper' AND a.status<>'deleted' LIMIT 1");
+ const channelName = String(q1("SELECT name FROM channels WHERE id=?", channelId)?.name || channelId);
+ if (skipper) await rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`,
+ { source: `1helm:channel:${channelId}:thread:${threadId}`, importance: 0.55, metadata: { kind: "channel-awareness", channel_id: channelId, thread_id: threadId, agent_id: agent.id }, sessionId: `channel:${channelId}` });
+ }
+ } catch (memoryError) {
+ console.warn("durable turn memory write failed:", (memoryError as Error).message);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await rememberForAgent(agent, episode, { source: `1helm:thread:${threadId}:message:${msgId}`, importance: 0.62, metadata: { kind: "session-outcome", channel_id: channelId, thread_id: threadId, message_id: msgId }, sessionId: `thread:${threadId}` }); | |
| if (agent.kind === "channel") { | |
| const skipper = q1("SELECT a.*, NULL channel_id FROM agents a WHERE a.kind='skipper' AND a.status<>'deleted' LIMIT 1"); | |
| const channelName = String(q1("SELECT name FROM channels WHERE id=?", channelId)?.name || channelId); | |
| if (skipper) rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`, | |
| if (skipper) await rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`, | |
| { source: `1helm:channel:${channelId}:thread:${threadId}`, importance: 0.55, metadata: { kind: "channel-awareness", channel_id: channelId, thread_id: threadId, agent_id: agent.id }, sessionId: `channel:${channelId}` }); | |
| try { | |
| await rememberForAgent(agent, episode, { source: `1helm:thread:${threadId}:message:${msgId}`, importance: 0.62, metadata: { kind: "session-outcome", channel_id: channelId, thread_id: threadId, message_id: msgId }, sessionId: `thread:${threadId}` }); | |
| if (agent.kind === "channel") { | |
| const skipper = q1("SELECT a.*, NULL channel_id FROM agents a WHERE a.kind='skipper' AND a.status<>'deleted' LIMIT 1"); | |
| const channelName = String(q1("SELECT name FROM channels WHERE id=?", channelId)?.name || channelId); | |
| if (skipper) await rememberForAgent(skipper, `Channel #${channelName}, resident @${agent.name}: ${episode}`, | |
| { source: `1helm:channel:${channelId}:thread:${threadId}`, importance: 0.55, metadata: { kind: "channel-awareness", channel_id: channelId, thread_id: threadId, agent_id: agent.id }, sessionId: `channel:${channelId}` }); | |
| } | |
| } catch (memoryError) { | |
| console.warn("durable turn memory write failed:", (memoryError as Error).message); | |
| } |
🤖 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 `@src/server/bots.ts` around lines 2002 - 2007, Isolate the two await
rememberForAgent calls in the turn-completion flow from the outer failure
handler so memory persistence errors cannot mark an already-rendered successful
turn as failed. Update the writes around the initial episode persistence and the
channel-awareness persistence inside the agent.kind === "channel" branch to
catch or otherwise contain their rejections, while preserving the existing
success bookkeeping and response behavior.
Comment on lines
1746
to
1757
| if (ociPrepareState.status === "running" && ociPreparePass) return channelComputerPrepareStatus(); | ||
| if (ociChannelImageExistsSync()) { | ||
| return setOciPrepareState({ | ||
| if (await ociChannelImageExists()) { | ||
| const state = setOciPrepareState({ | ||
| status: "complete", | ||
| step: "Channel computer image is ready. New channels can start immediately.", | ||
| progress: 100, | ||
| error: "", | ||
| started_at: ociPrepareState.started_at || now(), | ||
| }); | ||
| updateCachedOciImageState(true); | ||
| return state; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the prepare race created by the new await before the running guard.
Line 1746 reads ociPrepareState.status and ociPreparePass synchronously. Line 1747 then awaits ociChannelImageExists(). Two concurrent callers can both pass line 1746 while the state is still idle, both await line 1747, and both continue to line 1758. Each caller then sets the state to running and creates a pass. ociPreparePass retains only the second pass, so the first 30-minute image build runs unsupervised and is never cleared by the guard.
Set the running state before the first await, or re-check the guard after line 1747.
🛠️ Proposed fix
if (ociPrepareState.status === "running" && ociPreparePass) return channelComputerPrepareStatus();
if (await ociChannelImageExists()) {
+ if (ociPrepareState.status === "running" && ociPreparePass) return channelComputerPrepareStatus();
const state = setOciPrepareState({ const startedAt = now();
+ if (ociPrepareState.status === "running" && ociPreparePass) return channelComputerPrepareStatus();
setOciPrepareState({🤖 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 `@src/server/channel-computers.ts` around lines 1746 - 1757, Close the
concurrency race in the prepare flow around the initial running guard and the
awaited ociChannelImageExists() call. Update the relevant prepare function so
concurrent callers cannot both pass the idle-state check and create separate
preparation passes: either mark the state running before the first await or
re-check the running guard immediately after it. Preserve the existing
completed-image handling for callers that observe an available OCI channel
image.
Comment on lines
+1834
to
+1858
| function pendingRuntimeReadiness(): Record<string, unknown> { | ||
| const backend = configuredChannelBackend(); | ||
| const darwin = platform() === "darwin"; | ||
| const windows = platform() === "win32"; | ||
| if (backend === "oci") return { | ||
| backend, | ||
| supported: (["linux", "win32"].includes(platform()) && ["arm64", "x64"].includes(process.arch)), | ||
| engine_ready: false, | ||
| image_ready: false, | ||
| image: DEFAULT_CHANNEL_IMAGE, | ||
| prepare: channelComputerPrepareStatus(), | ||
| ready: false, | ||
| platform: platform(), architecture: process.arch, cli: null, version: null, system: null, | ||
| runtime_version: OCI_RUNTIME_VERSION, shared_runtime: windows ? installationScopedRuntimeName() : null, | ||
| storage_authority: windows ? `\\\\wsl.localhost\\${installationScopedRuntimeName()}\\var\\lib\\1helm-oci-v1\\runtime\\oci` : ociHostStateRoot(), | ||
| status: "checking", error: null, | ||
| }; | ||
| return { | ||
| backend, supported: darwin && process.arch === "arm64", darwin, arm64: process.arch === "arm64", | ||
| platform: platform(), architecture: process.arch, macos_version: null, cli: null, version: null, system: null, | ||
| runtime_version: APPLE_RUNTIME_VERSION, installer_url: APPLE_RUNTIME_URL, installer_sha256: APPLE_RUNTIME_SHA256, | ||
| status: "checking", error: null, engine_ready: false, image_ready: false, | ||
| image: DEFAULT_CHANNEL_IMAGE, prepare: null, ready: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
pendingRuntimeReadiness can throw on Windows during early startup.
Lines 1847 and 1848 call installationScopedRuntimeName(). That helper throws "1Helm installation identity is not ready." when the workspace.installation_id row is missing or malformed (src/server/channel-storage.ts lines 7-11). runtimeReadiness() calls pendingRuntimeReadiness() at line 2051 on every cache miss. On Windows, a readiness request before the installation identity is written therefore throws instead of returning the documented "request-safe snapshot".
Guard both fields.
🛠️ Proposed fix
+ let sharedRuntime: string | null = null;
+ try { sharedRuntime = windows ? installationScopedRuntimeName() : null; } catch { sharedRuntime = null; }
if (backend === "oci") return {
backend,
supported: (["linux", "win32"].includes(platform()) && ["arm64", "x64"].includes(process.arch)),
engine_ready: false,
image_ready: false,
image: DEFAULT_CHANNEL_IMAGE,
prepare: channelComputerPrepareStatus(),
ready: false,
platform: platform(), architecture: process.arch, cli: null, version: null, system: null,
- runtime_version: OCI_RUNTIME_VERSION, shared_runtime: windows ? installationScopedRuntimeName() : null,
- storage_authority: windows ? `\\\\wsl.localhost\\${installationScopedRuntimeName()}\\var\\lib\\1helm-oci-v1\\runtime\\oci` : ociHostStateRoot(),
+ runtime_version: OCI_RUNTIME_VERSION, shared_runtime: sharedRuntime,
+ storage_authority: sharedRuntime ? `\\\\wsl.localhost\\${sharedRuntime}\\var\\lib\\1helm-oci-v1\\runtime\\oci` : windows ? null : ociHostStateRoot(),
status: "checking", error: null,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function pendingRuntimeReadiness(): Record<string, unknown> { | |
| const backend = configuredChannelBackend(); | |
| const darwin = platform() === "darwin"; | |
| const windows = platform() === "win32"; | |
| if (backend === "oci") return { | |
| backend, | |
| supported: (["linux", "win32"].includes(platform()) && ["arm64", "x64"].includes(process.arch)), | |
| engine_ready: false, | |
| image_ready: false, | |
| image: DEFAULT_CHANNEL_IMAGE, | |
| prepare: channelComputerPrepareStatus(), | |
| ready: false, | |
| platform: platform(), architecture: process.arch, cli: null, version: null, system: null, | |
| runtime_version: OCI_RUNTIME_VERSION, shared_runtime: windows ? installationScopedRuntimeName() : null, | |
| storage_authority: windows ? `\\\\wsl.localhost\\${installationScopedRuntimeName()}\\var\\lib\\1helm-oci-v1\\runtime\\oci` : ociHostStateRoot(), | |
| status: "checking", error: null, | |
| }; | |
| return { | |
| backend, supported: darwin && process.arch === "arm64", darwin, arm64: process.arch === "arm64", | |
| platform: platform(), architecture: process.arch, macos_version: null, cli: null, version: null, system: null, | |
| runtime_version: APPLE_RUNTIME_VERSION, installer_url: APPLE_RUNTIME_URL, installer_sha256: APPLE_RUNTIME_SHA256, | |
| status: "checking", error: null, engine_ready: false, image_ready: false, | |
| image: DEFAULT_CHANNEL_IMAGE, prepare: null, ready: false, | |
| }; | |
| } | |
| function pendingRuntimeReadiness(): Record<string, unknown> { | |
| const backend = configuredChannelBackend(); | |
| const darwin = platform() === "darwin"; | |
| const windows = platform() === "win32"; | |
| let sharedRuntime: string | null = null; | |
| try { sharedRuntime = windows ? installationScopedRuntimeName() : null; } catch { sharedRuntime = null; } | |
| if (backend === "oci") return { | |
| backend, | |
| supported: (["linux", "win32"].includes(platform()) && ["arm64", "x64"].includes(process.arch)), | |
| engine_ready: false, | |
| image_ready: false, | |
| image: DEFAULT_CHANNEL_IMAGE, | |
| prepare: channelComputerPrepareStatus(), | |
| ready: false, | |
| platform: platform(), architecture: process.arch, cli: null, version: null, system: null, | |
| runtime_version: OCI_RUNTIME_VERSION, shared_runtime: sharedRuntime, | |
| storage_authority: sharedRuntime ? `\\\\wsl.localhost\\${sharedRuntime}\\var\\lib\\1helm-oci-v1\\runtime\\oci` : windows ? null : ociHostStateRoot(), | |
| status: "checking", error: null, | |
| }; | |
| return { | |
| backend, supported: darwin && process.arch === "arm64", darwin, arm64: process.arch === "arm64", | |
| platform: platform(), architecture: process.arch, macos_version: null, cli: null, version: null, system: null, | |
| runtime_version: APPLE_RUNTIME_VERSION, installer_url: APPLE_RUNTIME_URL, installer_sha256: APPLE_RUNTIME_SHA256, | |
| status: "checking", error: null, engine_ready: false, image_ready: false, | |
| image: DEFAULT_CHANNEL_IMAGE, prepare: null, ready: false, | |
| }; | |
| } |
🤖 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 `@src/server/channel-computers.ts` around lines 1834 - 1858, Update
pendingRuntimeReadiness for the OCI/Windows path so
installationScopedRuntimeName() is not invoked when the installation identity is
unavailable or malformed. Guard both runtime_scoped_name-dependent fields,
shared_runtime and storage_authority, returning safe null or fallback values
while preserving normal values once the identity is ready; ensure
runtimeReadiness continues returning its request-safe snapshot without throwing
during early startup.
Comment on lines
+2030
to
+2038
| if (runtimeReadinessPass) return runtimeReadinessPass; | ||
| const pass = probeRuntimeReadiness().then((value) => { | ||
| runtimeReadinessCache = { key, value, expiresAt: Date.now() + RUNTIME_READINESS_TTL_MS }; | ||
| return { ...value }; | ||
| }); | ||
| runtimeReadinessPass = pass; | ||
| const clear = (): void => { if (runtimeReadinessPass === pass) runtimeReadinessPass = null; }; | ||
| pass.then(clear, clear); | ||
| return pass; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A forced refresh can resolve with a probe that started before the caller's state change.
Line 2030 returns the in-flight pass to forced callers. The OCI probe uses timeouts up to 180 seconds (lines 1899 and 1908), so a probe can stay in flight for minutes. startAppleRuntime (lines 2098-2099) and beginOciChannelComputerPrepare (line 1769) both force a refresh immediately after they change runtime state. If a probe started before that state change is still running, the forced caller receives its pre-change result. startAppleRuntime then throws "Apple container runtime started but did not pass its health check." even though the runtime started.
Chain forced refreshes after the in-flight probe instead of returning it. Forced callers are rare, so this does not reintroduce a subprocess storm from UI polling.
🛠️ Proposed fix
- if (runtimeReadinessPass) return runtimeReadinessPass;
- const pass = probeRuntimeReadiness().then((value) => {
+ const inFlight = runtimeReadinessPass;
+ if (inFlight && !force) return inFlight;
+ const pass = (inFlight ? inFlight.then(() => probeRuntimeReadiness(), () => probeRuntimeReadiness()) : probeRuntimeReadiness()).then((value) => {
runtimeReadinessCache = { key, value, expiresAt: Date.now() + RUNTIME_READINESS_TTL_MS };
return { ...value };
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (runtimeReadinessPass) return runtimeReadinessPass; | |
| const pass = probeRuntimeReadiness().then((value) => { | |
| runtimeReadinessCache = { key, value, expiresAt: Date.now() + RUNTIME_READINESS_TTL_MS }; | |
| return { ...value }; | |
| }); | |
| runtimeReadinessPass = pass; | |
| const clear = (): void => { if (runtimeReadinessPass === pass) runtimeReadinessPass = null; }; | |
| pass.then(clear, clear); | |
| return pass; | |
| const inFlight = runtimeReadinessPass; | |
| if (inFlight && !force) return inFlight; | |
| const pass = (inFlight ? inFlight.then(() => probeRuntimeReadiness(), () => probeRuntimeReadiness()) : probeRuntimeReadiness()).then((value) => { | |
| runtimeReadinessCache = { key, value, expiresAt: Date.now() + RUNTIME_READINESS_TTL_MS }; | |
| return { ...value }; | |
| }); | |
| runtimeReadinessPass = pass; | |
| const clear = (): void => { if (runtimeReadinessPass === pass) runtimeReadinessPass = null; }; | |
| pass.then(clear, clear); | |
| return pass; |
🤖 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 `@src/server/channel-computers.ts` around lines 2030 - 2038, Update the runtime
readiness refresh flow around runtimeReadinessPass and the forced-refresh
callers so a forced refresh does not return an already-running probe started
before the state change. Chain the forced probe after the existing in-flight
pass completes, then cache and return the post-change probe result while
preserving deduplication for non-forced callers.
Comment on lines
+10
to
+18
| const sourceBetween = (source, start, end) => source.slice(source.indexOf(start), source.indexOf(end, source.indexOf(start))); | ||
|
|
||
| test("request-path subprocesses have no synchronous regression", async () => { | ||
| const memory = await readFile(join(root, "src", "server", "memory.ts"), "utf8"); | ||
| const computers = await readFile(join(root, "src", "server", "channel-computers.ts"), "utf8"); | ||
| assert.doesNotMatch(memory, /\bexecFileSync\b/, "the Python memory bridge must never synchronously stop Node's only thread"); | ||
| const readiness = sourceBetween(computers, "export function runtimeReadiness", "async function sha256File"); | ||
| assert.doesNotMatch(readiness, /\bspawnSync\b|ociChannelImageExistsSync/, "runtimeReadiness must only read cache and schedule asynchronous refreshes"); | ||
| assert.match(readiness, /void refreshRuntimeReadiness\(\)/, "an expired readiness snapshot schedules a refresh instead of becoming permanently stale"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm both slice markers exist and appear in the expected order.
rg -n 'export function runtimeReadiness|async function sha256File' src/server/channel-computers.tsRepository: gitcommit90/1Helm
Length of output: 286
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline test/event-loop-unblocking.mjs
printf '%s\n' '--- relevant test and imports ---'
sed -n '1,45p' test/event-loop-unblocking.mjs
printf '%s\n' '--- sourceBetween usages ---'
rg -n -C 2 'sourceBetween|runtimeReadiness|sha256File' test/event-loop-unblocking.mjs src/server/channel-computers.ts
printf '%s\n' '--- marker positions ---'
python3 - <<'PY'
from pathlib import Path
p = Path("src/server/channel-computers.ts")
s = p.read_text()
start = "export function runtimeReadiness"
end = "async function sha256File"
a = s.find(start)
b = s.find(end, a)
print({"start": a, "end": b, "ordered": a >= 0 and b > a})
PYRepository: gitcommit90/1Helm
Length of output: 12308
Assert that both slice markers exist and are ordered
Make sourceBetween assert that start and end are found, with end after start. Otherwise, renamed or reordered markers can make the regression assertions inspect the wrong slice.
🤖 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 `@test/event-loop-unblocking.mjs` around lines 10 - 18, Update sourceBetween to
validate that both start and end markers are present and that end occurs after
start before slicing the source. Assert or fail clearly for missing or reordered
markers, while preserving the existing slice behavior for valid markers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clicking a channel or thread took 10–15 seconds on a real Linux install. It wasn't the tunnel, the network, CPU, disk or query cost — the server made subprocess calls synchronously on the only thread that serves HTTP, so every request queued behind them.
The measurement
Sampling
/api/setup/status— an endpoint taking no arguments that normally answers in 1 ms — every 200 ms on loopback while the UI was in use:For that endpoint to take 21 seconds the event loop was blocked. Ruled out at the same time: server CPU 1.7%, a 10 MB bundle served in 20 ms,
sudo18–37 ms, 785 GB free disk.Root causes
1.
src/server/memory.ts—execFileSyncto the Python memory bridge. Blocked the loop for the full call: a 20-second timeout for most operations, 120 s for transcript sync, and a 771 ms floor just importingfastembed+sqlite_vec(measured on the affected host). The 21,178 ms stall is that 20-second timeout.Now async
execFile, preserving payload, env, both timeouts, buffer limit and return shape. The Python runtime probes went async too — they turned out to be reachable from the first memory operation on a request path, not only at setup.2.
runtimeReadiness()— aspawnSynccontainer-image check on request handlers including/api/computers, costing 53–73 ms of blocked loop every call. Now cached (5 s TTL, stale-while-revalidate, one shared in-flight refresh so concurrent requests can't cause a subprocess storm). Endpoints that act on readiness — setup completion, runtime start/prepare — await a fresh probe rather than trusting cache, and image preparation patches the cache directly so a stale "image missing" cannot persist.3. Duplicate
inspect—ensureOciProvisioned()already did the authoritative inspect; the caller repeated it immediately.Same defect as the Windows freeze
0.0.39 removed the boundary those calls crossed on Windows without removing the blocking, so Linux and macOS kept paying it. Fixing the blocking is what closes it for every platform.
Verification
recordMemory,buildContext,searchChannelHistory,syncChannelTranscript,runImprovementPassand ~18 call sites. Every call site of all 14 newly-async functions is awaited, voided or returned — verified by grep, because a droppedawaitwould silently lose a memory write and TypeScript won't catch it.test/event-loop-unblocking.mjs: structural bans plus a behavioural check that 10 ms timers keep ticking while simulated 250 ms subprocesses run, and that an uncached readiness read returns in under 50 ms.execFileSync, injecting a sync subprocess into readiness, and restoring the duplicate inspect each fail their test.npm run ci: 124 tests, 122 pass, 0 fail.Also fixed: a flaky test that kept obscuring this
test/native-world.mjsopened 38 of its own SQLite connections toctrl-pane.dbwhile the server held the same file, with no busy timeout — so it intermittently died withdatabase is locked. That fired four times today, including on CI for an unrelated PR. Test-side connections now wait up to 15 s for a lock, as a real client would. Five consecutive runs pass where the same build previously failed roughly one in three. No assertion changed.🤖 Generated with Claude Code
Summary by CodeRabbit