Code Review — PR #117: fix: daemon bugs — flush race, readline leak, timeline lock, client retry, file-request scope

What it does

Fixes five independent daemon bugs:

1. Readline leak — closes readline interfaces on process exit across all agent backends
2. Flush race — adds an isFlushing guard to prevent concurrent message batch flushes
3. Timeline lock — retries lock acquisition with short spin-waits before giving up
4. Client retry — initializes lastError with a descriptive Error so the throw path always has a meaningful message
5. File-request scope — scopes getRequest lookup by workspaceId so one workspace can't access another's file requests

Code Quality Checklist

Functionality

• Code does what it's supposed to do
• Edge cases handled
• Error handling appropriate
• No logic errors

Code Quality

• Readable and well-structured
• Functions are small and focused
• No code duplication
• Follows project conventions

Code Simplification

• Changes are minimal and targeted
• One concern: The spin-wait in timeline.ts (lines 157-161) uses a busy loop (while (Date.now() - start < 50 * (retry + 1)) {}) which blocks the Node.js event loop. Since this appears to be in a synchronous file-lock context, it's likely acceptable for 50-100ms, but worth noting. If this ever becomes a hot path, consider Atomics.wait or restructuring to async.

Security

• No security vulnerabilities
• Good catch: The workspaceId scoping on file requests closes a potential cross-workspace data access bug
• No hardcoded secrets

Tests Coverage

• No new tests added for these fixes. The flush race guard, timeline retry, and workspace-scoped query would benefit from unit tests, especially:
  • Concurrent flush prevention
  • Lock retry behavior (acquired on 2nd/3rd try vs. still failing)
  • getRequest with/without workspaceId

Project Syncing

• All three agent backends (claude, codex, opencode) updated consistently for rl.close()
• Both retry paths in client.ts updated

Observations

1. Flush race — lost messages on failure: When reportMessages throws, the spliced batch is silently lost (only logged at debug level). The previous code had the same issue, so this PR doesn't regress it — but it's worth considering a re-queue or at least a warning-level log for dropped messages.

2. Spin-wait justification: The spin-wait is a pragmatic choice for a sync file-lock path, but 50-100ms of event-loop blocking could stall other timers. Since this only triggers under contention (lock already held), it's probably fine in practice.

3. and(...conditions) — when workspaceId is undefined, conditions has only one element. Verify that and() with a single argument works correctly in your Drizzle version (it should, but worth a quick check).

Verdict

Solid set of targeted bug fixes. All changes are low-risk and improve correctness. Would be stronger with unit tests for the new behaviors, but the fixes themselves are clean and well-scoped. Looks ready to merge.