[SawyerHood]

Reproduction

A cold thread-list query on a 2.0 GB test database took 1.150 seconds. A concurrent health request took 1.098 seconds, and the server logged a 1.166-second event-loop stall.

Root cause

The thread-list handler ran synchronous better-sqlite3 reads on the server event loop. SQLite used temporary B-trees for the list query, so every client waited for the read to finish.

Fix

Move the complete thread-list read phase to a worker thread with a read-only database connection. Each worker operation uses one deferred read transaction.

The worker derives database-backed disconnected state. After the worker returns, the main thread applies current live daemon session state from the notification hub. This prevents stale host status during a slow read.

The same cold query now leaves the health route responsive at 1.2 ms. Slow-query logs return to the server logger, worker messages use validated contracts, and shutdown terminates the worker.

Fixes #1131

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am now reviewing this change for security, code quality, performance, architecture, and end-to-end behavior.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5

This change gives a slow database job to a separate worker. The main server can then answer other requests. The idea works for GET /threads, but the normal app uses a different route. Startup and worker failures can also leave the server in a bad state.

I recommend fixes before merge. I posted a comment-only review, as required.

Findings

• P1 — The normal app still uses the server event loop for thread-list reads. The app loads /sidebar-bootstrap. That route and projects?include=threads still run the thread queries and display queries on the server event loop. Move these complete read paths into the worker service.

• P1 — A later startup error can leave the process alive. runServer creates the worker before later operations that can fail. The top-level error handler only sets process.exitCode. Two reviews reproduced a process that did not exit. Close the worker on every startup error.

• P1 — A worker fault can permanently break thread lists. The service does not wait for a ready message. It stores worker errors and rejects all later list requests. The server can still report that it is ready. Require worker readiness before listener startup. Restart the worker or stop the server after an unexpected exit.

Architecture and security

I found no reusable server worker pool in this repository. Extend this database service for the sidebar and project read paths. The browser worker pool does not fit this server use.

The worker validates its messages and opens the database with read-only and query-only controls. I found no direct path-trust or data-exposure fault.

Validation

• Three GPT-5.6 workers reviewed security, code quality, architecture, and performance. One GPT-5.6 gate checked the combined result.
• Turbo type checks passed for @bb/server and @bb/db.
• Turbo tests passed for both packages. The server suite passed 1,368 tests in 159 files.
• The production server build passed and included the worker bundle.
• The source app started at the pull request SHA. A browser call to GET /api/v1/threads returned status 200.
• The browser received one seeded thread entry. The normal app displayed the same thread.
• git diff --check origin/main...HEAD passed. The worktree stayed clean.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and product behavior.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5

The server used to stop and read a large list by itself. This change gives that job to a helper. It is like asking a friend to count blocks while you answer the door. The idea works, but the helper can get stuck and accept too much work.

Findings

• High: A shutdown can leave a request pending forever during replacement worker startup. I reproduced this race at the required SHA. Reject every unresolved readiness promise during close(). Add a test for this shutdown point.
• Medium: The single worker queue has no limit, deadline, or client abort support. Slow repeated requests can consume memory and delay current reads. Add load control and tests for overload and disconnects.
• Low: The startup test allows a ten-second child wait inside a five-second Vitest test. The full local suite failed at five seconds. Give the test a longer timeout, or reduce the child wait.

Architecture

The worker boundary otherwise looks sound. It uses a read-only connection, strict messages, and a fixed worker entry path. I found no SQL injection or access-control regression.

The direct service and worker entry duplicate operation dispatch. The contract also repeats the daemon-session schema. A shared operation definition would reduce future drift.

Thread search still uses synchronous database work. I did not count that existing path as a defect in this pull request.

Validation

• The exact-SHA server and database type checks passed.
• The full server and database tests completed 1,372 of 1,373 tests. The new startup test hit its five-second timeout.
• The focused startup test passed. GitHub checks for the required SHA also passed during this review.
• Browser checks returned HTTP 200 for health, thread list, and sidebar routes.
• With 250,000 temporary rows, health returned in 2 ms while a 278 ms list request still ran.
• A 10,000-row response kept the maximum observed health latency near 30 ms.

The main event-loop goal works. The shutdown race needs a fix before merge. I left three inline comments with specific changes.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am now reviewing this pull request under the configured security, code quality, performance, architecture, and end-to-end guidelines.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5

The app gives a slow list job to a helper. It works like asking a friend to count toys while you keep playing.

Review

The main idea works. The database query now runs outside the server event loop. I found these issues:

1. High: A large limit value fails worker validation before the request gets an ID. The worker exits and rejects unrelated reads.
2. High: An active timeout settles the promise but leaves activeRead set. Later reads wait or time out until the query returns.
3. Medium: The main thread validates every returned entry with Zod. Tests measured about 315 ms for transfer decode and validation of 100,000 rows.
4. Medium: The service marks queue and timeout errors as retryable 503 responses. The app retry policy rejects all HttpError values.
5. Medium: A client abort creates a plain Error. The server then records an error log and maps the error to HTTP 500.

The read-only SQLite connection has the correct write controls. Existing project and thread access checks still run before worker reads.

The architecture scan found no reusable server worker pool, so this service does not duplicate an existing worker layer. However, the Zod schema repeats ListThreadsOptions. Add a key coverage guard to prevent silent contract drift.

The route and integration harnesses use the direct service. Thus, route tests do not test the production worker boundary. Add one route test with the worker and a full thread entry.

Validation passed:

• Server typecheck
• Server production build
• 14 focused server tests
• 5 database tests
• Local UI load with the real worker
• HTTP 200 from /api/v1/sidebar-bootstrap and /api/v1/threads?limit=10
• No database worker errors in the local server log

I left this as a comment review. I did not approve the pull request or request changes.

[SawyerHood]

Review follow-up for 3bdc475:

• Moved full result validation into the worker.
• Added safe HTTP bounds and request-local worker validation errors.
• Replaced the worker after an active read timeout.
• Added compile-time option-key coverage and a production-worker route test.
• Kept the 503 retry behavior unchanged. The maintainer deferred that product decision.
• The abort and response-byte findings were fixed in 14573f0.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5

This change gives slow list work to a helper. It is like asking a helper to sort cards. The main app can keep moving.

Two stop rules can fail. The helper can then block new work or return an error that the app will not retry.

Findings

I found four issues:

• High: An active request abort clears its timer but leaves the worker busy.
• High: An unexpected worker failure returns a non-retryable error, although the service starts a replacement.
• Medium: A slow read can trigger five 30-second attempts and five worker replacements.
• Medium: Sidebar bootstrap sends two related reads into one serial queue.

The inline comments contain the impact and a concrete fix for each issue.

The sidebar should send one request for all project identifiers. This refactor removes duplicate queue work and gives one result boundary.

I found no authorization, data exposure, injection, or unsafe path issue. The active-abort issue can cause an availability failure.

Validation

• I reviewed all 27 changed files at the stated commit.
• git diff --check passed.
• Turbo type checks passed for the app, server, SDK, and database packages.
• The focused worker test file passed all 12 tests.
• The local full run passed 1,383 of 1,385 tests.
• The relevant timeout test passed alone. The other local failure came from an unrelated plugin test.
• All GitHub checks passed.
• Browser QA created an isolated project and thread.
• The sidebar showed both records.
• The thread page opened successfully.
• All observed thread and list routes returned status 200.

I would fix the two high findings before merge. I left a comment review only. I did not approve or request changes.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5: This change gives slow thread-list database work to a background worker. The main server can then answer other requests.

I reviewed a0af660ae9. The pull request advanced to 938458ad24 during the review. That later commit only changed test synchronization.

I found three medium-severity issues:

1. The project routes read projects before the worker await and defaults after it. A concurrent deletion can produce an impossible sidebar response.
2. Each worker operation runs several SQLite reads without a transaction. A write can make one response combine data from different WAL snapshots.
3. Each active client cancellation replaces the worker. Repeated request cancellations can cause worker churn and deny thread-list service.

The first two issues need one consistent read transaction. The project and sidebar operations must move all response reads into that transaction.

For cancellation, reject the caller but keep the worker request active. Discard its result, and replace a worker only after an internal timeout.

The reviewed SHA also had an intermittent server test failure. Commit 938458ad24 added a replacement-ready wait, and the new server CI run passed.

I found no reusable server worker framework. The dedicated service fits the current architecture.

The direct and worker adapters repeat the operation dispatch. A shared operation function can remove that duplication.

Validation passed locally:

• The full server test run passed with 1,388 tests.
• The server Turbo typecheck passed.
• The server Turbo build passed and included the worker artifact.
• The browser loaded the sidebar without console errors.
• The thread list, project list with threads, and sidebar bootstrap routes returned valid responses.
• git diff --check passed.

I left this as a comment review. I did not approve the pull request or request changes.

[SawyerHood]

🚨 slopcop/review — Run the complete worker operation in one read transaction.

Reviewed line

This query and toThreadListEntryResponses run several database reads. A main-thread write can commit between those reads.

The result can combine thread rows and activity rows from different WAL snapshots.

Use one deferred read transaction for each worker request. This transaction will pin one consistent SQLite snapshot.

[SawyerHood]

🚨 slopcop/review — This await breaks the sidebar response's database snapshot.

Reviewed line

The route builds projects and sources before this await. It reads defaults after the await.

I reproduced a concurrent deletion during this wait. The response included the deleted project without its former thread.

That response matched neither the state before deletion nor the state after deletion.

Move the complete sidebar read into one worker-side read transaction. Include projects, sources, sections, defaults, and threads in that transaction.

[SawyerHood]

🚨 slopcop/review — Do not replace the worker when a client cancels an active read.

Reviewed line

Each active cancellation starts a new worker and stops the current worker. The queue limit does not bound this restart path.

A client can repeat a list request and close its socket after dispatch. This can deny service to other list requests.

Reject the caller, but keep the active slot until the worker returns. Discard that result and reserve worker replacement for internal timeouts.

[SawyerHood]

Addressed all three findings from SlopCop run run_6vklc90omskvazsv in ba8543a.

• Each worker operation now uses one deferred SQLite read transaction.
• The project and sidebar worker operations now read projects, sources, defaults, sections, and threads in that transaction.
• An active client cancellation now rejects the caller but lets the worker finish. Only an internal timeout replaces the worker.
• A concurrent-delete test confirms that a sidebar response keeps one database snapshot.
• A worker-count test confirms that an active cancellation does not replace the worker.
• A route test confirms that the worker and direct sidebar paths return identical bytes.

All app, database, SDK, and server typechecks pass. The app, database, and SDK suites pass. The server suite passes 1,389 tests and has only the known 0664-versus-0644 skill-file failure.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior.

[SawyerHood]

🚨 SLOP COP 🚨 · review

ELI5: The server now gives large thread-list database work to a helper. The main server can answer other work during that read.

I found no security defects. I found three concrete reliability and performance defects.

1. High: A list with 16,377 visible threads still fails with SQLite's variable limit. The background-task query binds every thread ID twice.
2. Medium: The worker can return old host status. It records daemon sessions before the database read finishes.
3. Medium: Worker recovery calls startWorker() without a guard. A direct constructor error can stop the server, and readiness has no deadline.

The change improves event-loop availability, but it does not reduce the database cost. One slow read still blocks the single worker queue.

The active read also continues after a caller aborts. This keeps the worker busy until the query finishes or reaches its deadline.

The thread search route still runs its database read on the main event loop. This route remains outside the new service.

The project response code now repeats source grouping in two files. A shared pure helper can prevent future response drift.

Validation completed at exact SHA ba8543a7b61136154d2f0bcb75a7a4dee4828db5:

• The server, app, database, and SDK type checks passed.
• The production server build passed and included the worker entry.
• All 15 worker-service tests passed in an isolated rerun.
• The route, database, SDK, and retry test groups passed.
• The full parallel run passed 1,389 of 1,390 tests. One worker test reached its time limit under heavy host load.
• The same worker test passed alone in 3.6 seconds.
• The exact dev server returned valid sidebar and thread-list responses.
• A headless browser loaded the thread-list route without page errors.

I left inline notes on the three concrete defects. I did not approve or request changes.

[SawyerHood]

🚨 SLOP COP 🚨 · review

I am the SlopCop. I am reviewing this pull request for security, quality, performance, architecture, and user behavior.

I will send the checks to three parallel reviewers. I will post one final review after the checks finish.

[SawyerHood]

Closing this in favor of #1204.

This PR answered the 11.2 second event-loop stall in #1131. A measurement pass
(#1204) then showed the stall came from listLatestGoalEventRowsByThreadIds,
not from the thread-list query. A partial index cut that query from 10.5s to
0.10s cold. A worker thread moves a slow read off the loop; it does not make the
read fast. #1204 makes it fast, so the reason for this machinery is gone.

The work here was good and the review found real problems, which is exactly why
it is worth being deliberate about adopting it. The residual cost is recorded
with its measurements in #1207 (Task, Low), so this option stays revivable. The
branch is kept.

Two pieces of this PR outlive it:

• The retryable finding is a pre-existing bug on main, independent of this
  change. retryable: true is produced today (for example
  apps/server/src/services/threads/thread-create.ts:146 and several
  host-daemon contract errors) and no client code reads it. That deserves its
  own PR.
• The listActiveBackgroundTaskCountsByThreadIds variable-limit crash found
  during this work is filed as listActiveBackgroundTaskCountsByThreadIds throws 'too many SQL variables' at ≥16,377 visible threads, failing the thread-list route #1205.

Thanks for the thorough work on this one.