docs(storage): investigate runtime database VACUUM placement

[garvit-arora]

Summary\n\nDocuments the requested #6000 placement analysis before any implementation: Host writer interaction, the 75 s startup / 8 s liveness constraints, recovery and free-space behavior, an explicit recommendation, and the smallest first PR. Adds a reproducible Node SQLite fixture and manual Windows/Ubuntu benchmark workflow.\n\nThe in-process Windows measurements and the historical #5855 production-derived measurements are kept distinct in the doc. Ubuntu fixture results will be added when the workflow completes.\n\nCloses no issue; maintainer decision requested.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed range: merge-base c11a533 (main) to head bd1ca5b, 1 commit, 3 files, +165/-0 (design doc, benchmark script, manual workflow). This is a first review, so there are no prior findings. No protocol files are touched, so there is no epoch impact.

Verdict: one P2 (PR-caused CI failure, plus a header failure that will follow), and three P3 notes on factual accuracy in the doc and harness. The recommendation itself (after-Ready worker plus a Host-owned writer gate, with report-only as the fallback) is reasonable.

P2: CI test fails on the new workflow. scripts/ci-workflow-policy.test.mjs rejects runtime-vacuum-benchmark.yml twice (job 113832831773):

• "ubuntu-latest queues for a runner; name the image instead". The repo uses ubuntu-24.04.
• "workflows never persist the job credential into the checkout". The checkout step needs with: persist-credentials: false, which the repo's other 37 checkouts set.

Once those pass, check:asf-headers will fail next. I ran it locally: .github/workflows/runtime-vacuum-benchmark.yml and docs/design/runtime-database-vacuum-placement-6000.md are missing the ASF header (npm run write:asf-headers).

Claims checked against code (correct):

• runtime.sqlite is WAL (sqlite-runtime-schema.ts:952).
• Production source has no auto_vacuum/VACUUM, so the database is NONE.
• The runtime store sets a 5 s busy timeout (sqlite-runtime-store.ts:238).
• The startup deadline is 75 s (connect-or-spawn.ts:75, wait-for-ready.ts:24).
• Client liveness is 8 s (connection.ts:97).
• The ~55 s before-listen and ~13 s/GiB after-Ready figures match #6000. #5855 merged with runtime compaction deferred to a follow-up.

P3: "the existing Compact database action" doesn't exist on main (doc lines 7 and 48). No such operation, UI string, or protocol entry exists. #6000 lists it as something #5825 agreed future work would use. Call it the planned explicit request, so the "smallest first PR" scope includes adding the action and its protocol operation (with the epoch that implies).

P3: the timer-delay column isn't an independent measurement. clearTimeout(timer) runs synchronously right after conversion.exec('VACUUM'), so the timer callback can never fire. mainThreadTimerDelayMs is therefore always max(0, vacuumMs - 50). The table shows this (0.93 to 0.88, 10.17 to 10.12, 36.18 to 36.13), and my local 50 MiB run gave 320 ms and 270 ms. Either drop the column or measure real delay by yielding before clearing the timer. Since the conversion would run on the main thread, the conclusion that a 1 GiB conversion would stall the event loop past 8 s still holds.

P3: the fixture doesn't match production journal mode. The script sets journal_mode=DELETE, but runtime.sqlite is WAL. In WAL mode, VACUUM writes the rewritten database through the WAL. That changes both timing and peak disk use (the WAL can grow to about the size of the database until a checkpoint). That matters for the doc's 2x free-space preflight, so either benchmark with WAL or state the difference.

Minor: a workflow_dispatch workflow can only be dispatched once it exists on the default branch. The "Ubuntu numbers appended after that run completes" step therefore needs a fork run or a merge first.

Mergeability/CI: GitHub reports MERGEABLE, but the merge state is BLOCKED (no approving review). test fails (PR-caused, above); label passes.

Update for head fc896403: while this review was being prepared, fc896403 added canonical license headers to the workflow and the design doc, so the header-check part of the P2 is addressed. The workflow-policy part is still open on this head: the matrix still uses ubuntu-latest, and the checkout step still has no persist-credentials: false. The inline comments have been moved to the shifted lines.

[Astro-Han]

P2: ci-workflow-policy.test.mjs rejects ubuntu-latest ("name the image instead"). The repo uses ubuntu-24.04. This file also needs the ASF header.

[Astro-Han]

P2: the policy test requires with: persist-credentials: false on every checkout.

[Astro-Han]

P3: this runs synchronously right after exec('VACUUM'), so the timer can never fire and mainThreadTimerDelayMs is always vacuumMs - 50. It isn't an independent measurement.

[Astro-Han]

P3: runtime.sqlite runs in WAL mode (sqlite-runtime-schema.ts:952). With journal_mode=DELETE here, the benchmark misses VACUUM's WAL write and disk-growth behavior.

[Astro-Han]

P3: there's no existing Compact database action on main (no operation, UI, or protocol entry). It's planned per #5825/#6000, so the first PR also needs to add it.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: fc89640 to 1d8124a. This adds two commits: 23a6565 (fixes) and 1d8124a (formatting only). The merge-base is unchanged (c11a533) and there was no rebase. The delta touches the workflow, the design doc, and the benchmark script.

Prior findings

• P2 (CI test failed in ci-workflow-policy.test.mjs): fixed.
  • The matrix now uses ubuntu-24.04.
  • The checkout step sets persist-credentials: false.
• P3 ("existing Compact database action"): fixed.
  • The doc now calls it the planned action and says the action and its protocol operation do not exist yet.
  • The first-PR scope now includes adding the action and protocol request, and claiming the compatibility epoch at merge time.
• P3 (the timer-delay column was not an independent measurement): fixed. The timer and the column are removed, and the doc now says the synchronous duration is the block time.
• P3 (the fixture used DELETE instead of production WAL): fixed.
  • The harness now uses journal_mode=WAL. The final db.close() checkpoints and removes the WAL before cpSync, so the copied file is complete.
  • The table was re-run in WAL mode, and the doc states that peak WAL growth is not measured.
  • The free-space preflight now covers the whole database file set, including the WAL.
  • I ran the updated harness locally (Linux, SQLite 3.51.3) at 20 and 50 MiB: 130 ms and 276 ms. It works.

New findings in this range

P3: One stale number in the "Before listen" row of the placement table (doc line 54). It still says "The 3 GiB fixture is 36 s on this Windows host". That figure is the excluded DELETE-mode result; the WAL table now shows 140.36 s. The correction strengthens the argument against before-listen placement, since a 3 GiB conversion alone exceeds the 75 s startup deadline. Please update the sentence.

No protocol or epoch files are touched. The doc now says the future action will need an epoch.

Mergeability/CI: test passes on 1d8124a. GitHub reports the PR as MERGEABLE, and the merge state is BLOCKED only because there is no approving review. No P0-P2 issues remain.

[Astro-Han]

P3: "The 3 GiB fixture is 36 s on this Windows host" is the excluded DELETE-mode figure. The WAL-mode table above now reports 140.36 s for 3,072 MiB, which on its own exceeds the 75 s startup deadline. Please update this sentence to match.

[liugddx]

Thanks for the thorough write-up. The constants check out against main: the 75 s startup deadline (connect-or-spawn.ts:75, wait-for-ready.ts:24), the 8 s liveness timeout (client/connection.ts:97), WAL with synchronous=FULL and the 5 s busy_timeout. Astro-Han's earlier points are addressed, except that line 54 still quotes "3 GiB 36 s", which is the excluded DELETE-mode result; the WAL run is 140 s. Before this can support a placement decision, a few gaps need closing:

1. The ownership model. runtime.sqlite is served through a single process-local owner connection, and repositories get leases rather than opening connections (operational-state-store.ts:180-184). Secondary processes can also open it with require_current, for example session-bundle-policy.ts:454 and storage-footprint.ts:131. A worker with its own second connection departs from that model, and a Host-only gate doesn't give exclusivity across processes.
2. What the worker actually buys. VACUUM holds the write lock for its whole run, so concurrent writers hit SQLITE_BUSY after 5 s either way. Moving it to a worker only keeps the event loop and reads alive; the doc should say so plainly.
3. Disk and timing. The WAL grows to roughly the database size during conversion, so the free-disk preflight is closer to 3× than 2×. The harness also stops timing before close(), which leaves out the final checkpoint, and never asserts auto_vacuum = 2 and an empty freelist afterwards. The 1 → 3 GiB jump (10.7 s → 140 s) is super-linear and needs an explanation (WAL growth, page cache, AV scanning).
4. Missing options. VACUUM INTO a staging file needs only a read transaction and leaves the live database untouched on crash. Swapping the file at the next start, before listen, avoids the long exclusive write lock (Windows can't rename open handles, so it can't be swapped live). The repo already vacuums a private copy rather than the live file in context-offload-snapshot.ts:163-166. Also spell out crash recovery: under WAL with FULL, a VACUUM without a commit frame is discarded and the original database survives intact.
5. The smallest first PR is smaller than proposed. New runtime.sqlite databases can be created with auto_vacuum = INCREMENTAL before the schema and WAL exist, which needs no VACUUM at all. sqlite-context-offload-schema.ts:218-223 already does exactly this. Existing databases are a separate, later step.
6. Two platforms. #6000 asked for measurements on two platforms; only Windows is measured.

On packaging: please don't add a one-off benchmark workflow to apache/maka. It only has to exist on your fork's default branch for you to dispatch it there. I'd also rather not start a docs/design/ directory for this. Could you post the conclusions on #6000 (or keep the harness under scripts/perf/ with a short README) and close this PR? Once a placement is agreed, the decision can go into docs/architecture/ with the implementation.

[garvit-arora]

Closed per the request on #6000: the corrected benchmark and option analysis are now on the issue, and the harness notes are under scripts/perf/README.md. Existing-database placement remains undecided pending a second-platform run and cross-process ownership design.