feat(storage): reclaim disk space for context-offload and runtime databases - #5855
garvit-arora wants to merge 2 commits into
Conversation
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
This adds bounded context-offload and runtime SQLite page reclamation, a restart-time runtime compaction request, protocol operations, and a Data Settings control. I found three correctness issues in the new paths (inline). The current head is not ready to use for storage-reclamation reporting or the new Settings control.
Node 24 clean install, build:test, Desktop typecheck, seven focused tests, protocol epoch guard, ASF headers, app-shell hook check, diff check, and a merge against current main passed. Reproductions used the built journal and page-reclamation modules. A focused Biome check found five formatting differences; hosted checks currently show only the label job. I did not run a packaged Desktop or a full end-to-end Host restart.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [host, locale, props.runtimeHostTargetVerified, toast, copy.storageLoadFailed, diagnosticTarget]); |
There was a problem hiding this comment.
[P1] Stop re-querying on every render. diagnosticTarget is a fresh object on every render (line 85), and this effect lists it as a dependency. Each successful queryReport calls setStorageReport with a new result, which renders again, changes that dependency, and immediately issues another Host query. With Data Settings open this creates an unbounded request loop; failures can similarly re-toast. Depend on the stable profile ID instead, or construct the diagnostic target only in the error path.
| journal: StorageReclamationJournalV1, | ||
| ): Promise<void> { | ||
| const path = resolveStorageReclamationJournalPath(workspaceRoot); | ||
| const tempPath = `${path}.tmp`; |
There was a problem hiding this comment.
[P2] Serialize journal updates and use a unique temp file per write. The context and runtime maintenance lanes run independently, and both call recordStorageReclamation; the UI command also calls requestRuntimeDatabaseCompaction. All three perform an unsynchronized read/modify/write through this same .tmp path. In the built module, concurrent context/runtime records consistently produced an ENOENT rename and lost one count. Concurrent compaction request + context record also produced a fulfilled request whose final journal had no runtimeCompact flag, so the UI can say “scheduled” while the next Host start does nothing. Preserve both fields atomically under a per-root lock (or equivalent), and cover overlapping writers.
| }; | ||
| } | ||
|
|
||
| export function runPassiveWalCheckpoint(db: DatabaseSync): void { |
There was a problem hiding this comment.
[P2] The passive checkpoint leaves the WAL file allocated, so the reported reclaimed bytes are not disk space actually returned. With this built helper on an incremental WAL database holding an 8 MiB blob, runBoundedIncrementalVacuum reported 8,396,800 reclaimed bytes; after the passive checkpoint, the main database was 12,288 bytes but -wal remained 6,493,152 bytes, so only 1,911,840 bytes of the file set were freed while the store stayed open. The Data Settings total accumulates the larger number. Account for the whole database/WAL file set and safely truncate/checkpoint at a quiescent point, or label the metric as pages rather than disk space reclaimed.
|
Thanks for the review — addressed all three items in P1 — Data Settings query loop
P2 — Journal write races
P2 — WAL / reclaimed-byte accounting
Verification
Ready for another look. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 2e7c4a97019d7ffa6f8012bb8414593c439d544c (incremental from c9529555). The fix commit resolves the Data Settings re-query loop and the concurrent journal writes (queued per root, unique temp file; 0 failures in 200 concurrent runs locally). I found no path that deletes user rows — reclamation only frees empty pages / runs VACUUM, and clients cannot choose a path. It is still not merge-ready: the new checkpoint regresses context-offload reclamation, and the startup compaction path and CI guard remain open.
P2 (new in this commit)
- Context-offload page reclamation now always fails: the TRUNCATE checkpoint runs inside the store transaction (inline).
runReclamationWalCheckpointcan hold the Host's main thread for the full busy timeout per tick while another connection has a reader (inline).
P2 (still present)
check:renderer-architecturefails: the legacysettings/directory gains new bridge calls and hooks (inline). Only thelabelcheck has reported on this PR so far, so this has not shown up in CI yet.- Requested runtime compaction runs a full blocking
VACUUMduring Host startup before it listens (inline). Locally a 1.03 GB runtime DB took ~55 s against the 75 s default startup deadline; a failed/interrupted attempt leaves the request pending so it reruns each start. The space check requires only 1× the DB file-set size (inline), but the WAL grew to ~1× during the run on top of SQLite's temp copy, and the raw connection has no busy timeout. - An empty or corrupt journal file makes read/request/record throw on every call (
JSON.parsefailure is rethrown), so the status query fails and the maintenance lane logs every tick until the file is deleted by hand; writes are also not fsynced beforerename(inline).
P3
- WAL truncation after ordinary writes is counted as reclaimed bytes (1.24 MB measured with zero pages freed), so the Settings total grows with normal use.
- Failures only reach the log; the page keeps showing "scheduled for next start".
- Epoch 198→199 is the right call, but
protocol-compatible-changes/storage-reclamation-operations.jsonis then redundant and its rationale is inaccurate (a new client can send the new operation to an older Host). - Context-offload
reclaimFreePageswrites inside#readTransaction; the runtime maintenance lane opens a lease on every tick outside the shared transaction API. git diff --checkfails on a trailing blank line at the end ofsqlite-page-reclamation.ts.- Runtime compaction, both maintenance lanes' byte accounting and the renderer/IPC wiring have no tests; none of the above would be caught.
Checks run locally: ASF headers, epoch check (198→199), Biome on changed files, Desktop typecheck, storage 71/71 and runtime-host 103/103 targeted tests (pass); check:renderer-architecture and git diff --check fail. The checkpoint, stall, journal-corruption and VACUUM timing findings were reproduced against the built modules. Not verified: whether Desktop kills the Host when the startup deadline expires mid-VACUUM, and Windows behaviour.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| const result = this.#readTransaction(() => { | ||
| this.#assertOpen(); | ||
| const vacuum = runBoundedIncrementalVacuum(this.#database, input.maxPages); | ||
| runReclamationWalCheckpoint(this.#database); |
There was a problem hiding this comment.
P2: PRAGMA wal_checkpoint(TRUNCATE) inside the open transaction is rejected by SQLite ("database table is locked"); the error rolls back the incremental vacuum too. Against a real store both calls threw and the 639 free pages were still there afterwards, so this lane now reclaims nothing and retries indefinitely. Run the checkpoint after the transaction commits.
| } | ||
|
|
||
| export function runReclamationWalCheckpoint(db: DatabaseSync): void { | ||
| const row = db.prepare('PRAGMA wal_checkpoint(TRUNCATE)').get() as { |
There was a problem hiding this comment.
P2: TRUNCATE waits on readers in other connections up to the busy timeout — measured 5,006 ms on the Host's main thread. The runtime lane calls this every tick and repeats every 100 ms while hasMore, so a long-lived reader keeps the Host almost continuously blocked. Consider PASSIVE here (or a zero busy timeout for this call) and leave TRUNCATE for idle/explicit compaction.
| return; | ||
| } | ||
| let cancelled = false; | ||
| void window.maka.storageReclamation.queryReport(host).then((result) => { |
There was a problem hiding this comment.
P2: check:renderer-architecture fails on this head — src/renderer/settings is a legacy no-growth directory, and this adds new window.maka bridge calls plus a useEffect/useState. Move the query behind a feature/application-zone hook or update renderer-architecture.json with justification.
| ): Promise<ExecutionRuntimeHostComposition> { | ||
| const workspaceRoot = context.owner.capability.canonicalPath; | ||
| try { | ||
| await runRequestedRuntimeDatabaseReclamation(workspaceRoot); |
There was a problem hiding this comment.
P2: this awaits a full VACUUM before the Host starts listening. ~55 s for a 1.03 GB DB locally vs the 75 s default startup deadline; if the attempt fails or is killed, the request stays pending and reruns every start. Consider running it after listen (with progress/timeout) and clearing or backing off the request on failure.
| } | ||
| const databaseSize = await readSqliteDatabaseFileSetBytes(databasePath); | ||
|
|
||
| const availableBytes = await readAvailableBytes(databasePath); |
There was a problem hiding this comment.
P2: 1× the DB file-set size underestimates what VACUUM needs — the WAL grew to ~1× during the run plus SQLite's temp copy, so ~2× is the realistic floor. The raw connection used here also has no busy timeout, so a concurrent writer fails the attempt immediately.
| try { | ||
| const raw = await readFile(path, 'utf8'); | ||
| const parsed = JSON.parse(raw) as StorageReclamationJournalV1; | ||
| if (parsed.version !== 1) return undefined; |
There was a problem hiding this comment.
P2: a truncated/empty or otherwise unparseable journal (e.g. power loss — writes are not fsynced before rename) makes every read, request and record throw from here, so the report query fails and both maintenance lanes log each tick until the file is removed by hand. Treat parse failure like a missing journal (optionally quarantine the file) and fsync the temp file before renaming.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the new head independently after the prior c9529555 review. The Data Settings query effect no longer depends on the per-render diagnostic-target object (data-settings-page.tsx:85,132), and journal read-modify-write operations are now serialized within this process with unique temporary files (storage-reclamation-journal.ts:59-133). The new main/WAL/SHM file-set measurement avoids the previous page-count overstatement, but it is not a measure of pages actually reclaimed: with zero freelist pages, a checkpoint-only probe still reported 22,704 reclaimed bytes.
No additional distinct inline findings beyond the existing current-head review by Astro-Han (5357393426). I independently reproduced its context-offload regression: with 257 free pages, SqliteContextOffloadStore.reclaimFreePages throws database table is locked at the checkpoint inside #readTransaction (sqlite-context-offload-store.ts:523-527), leaving all pages free. The startup compaction remains on the pre-ready composition path (execution-composition.ts:340-348), and the architecture guard remains red on the new Settings bridge/hook usage. This head is not ready to merge.
Validation: Node 24 build:test, eight focused storage/Host tests, ASF headers, app-shell hooks and a clean merge-tree against current main b3933043 passed. check:renderer-architecture fails on two new bridge calls and one additional hook/state; Biome reports four changed-file formatting failures; git diff --check reports a trailing blank line in sqlite-page-reclamation.ts. No hosted checks are reported for this head yet. I did not run a packaged Desktop or a full-size database compaction. No schema migration is introduced.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Follow-up on the second review round in P2 — context-offload regression
P2 — blocking TRUNCATE checkpoint
P2 — renderer architecture
P2 — startup compaction / journal
Tests
Ready for another look. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head fa023f713e09c88d47f9953ca88caf64b9eca41e (incremental from 2e7c4a97). Thanks for the quick turnaround — most of the earlier findings are resolved:
- Context-offload checkpoint now runs after COMMIT; against a real store with 639 free pages a tick reclaimed 64 pages (575 left).
- Periodic ticks use only PASSIVE; with another connection holding a read transaction a tick now takes ~0 ms (was ~5 s).
- Unparseable journal is treated as missing, and writes fsync file + directory.
- Ordinary WAL shrink is no longer reported as reclaimed (300 plain writes → 0 bytes).
check:renderer-architectureandgit diff --checkpass. (The guard passes by raising the allowed counts inrenderer-architecture.jsonrather than moving the code out of legacysettings/; that is a maintainer call.)
Remaining P2 — startup compaction (inline): the free-space check now requires 2× and the connection has a busy timeout, but the full VACUUM still runs before the Host listens, with no progress or deadline, and a failed or killed attempt stays pending and reruns on every start. SQLite's temp copy location (OS temp dir) is also not checked.
P3
lastReportshape is not validated beyondversion; a malformed report passes through to the client decoder, which rejects it.- Compaction failures still only reach the log, so Settings keeps showing "scheduled".
- The compatible-change declaration is redundant with the 198→199 epoch bump; the vacuum still runs under
#readTransaction; the runtime maintenance lane opens a lease every tick. - Tests remain thin: the new store test asserts only
reclaimedPages >= 0, and runtime compaction / runtime maintenance have no coverage.
Checks run locally: renderer-architecture, git diff --check, ASF headers, epoch check, Biome on changed files, storage 73/73 and runtime-host 103/103 targeted tests — all pass.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| ): Promise<ExecutionRuntimeHostComposition> { | ||
| const workspaceRoot = context.owner.capability.canonicalPath; | ||
| try { | ||
| await runRequestedRuntimeDatabaseReclamation(workspaceRoot); |
There was a problem hiding this comment.
P2: this still awaits a full blocking VACUUM before the Host starts listening. Locally ~55 s per GB, against 75 s / 45 s startup deadlines; if the attempt fails or the Host is killed at the deadline, the request stays pending and reruns every start. Consider running compaction after listen (or in a worker) with progress and a deadline, and clearing or backing off the request after a failed attempt.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head fa023f713e09c88d47f9953ca88caf64b9eca41e. The checkpoint-outside-transaction fix works, and periodic checkpoints no longer block on a WAL reader. One additional P2 remains: the maintenance report now counts logically vacated SQLite pages as physical bytes reclaimed even when a live WAL reader prevents any file-set shrink. This is separate from the startup-compaction blocker already reported in the current-head review. Node 24 build:test, 29 focused tests, renderer architecture, ASF headers, and merge-tree/diff check passed; Biome formatting reports three files, and hosted checks are not yet available. I did not test packaged Desktop or multi-GB compaction.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| this.#assertOpen(); | ||
| return runBoundedIncrementalVacuum(this.#database, input.maxPages); | ||
| }); | ||
| runPassiveWalCheckpoint(this.#database); |
There was a problem hiding this comment.
P2: runBoundedIncrementalVacuum returns reclaimedPages * pageSize, but the new PASSIVE checkpoint cannot truncate the WAL while another connection has a read transaction. In a real Node 24 WAL probe with a second reader held open, this path reported 1,026 reclaimed pages / 4,202,496 bytes while the database+WAL+SHM file set remained exactly 8,544,720 bytes before and after. storage-maintenance.ts records this as reclaimed disk space (the runtime maintenance path does the same). Please keep logical page reclamation separate from physical bytes freed, and report a physical reclaim only when the file set actually shrinks; add a held-reader regression.
|
Round 3 review feedback addressed on latest push:
Ready for another look. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e7ff8df8daf84af477f75a143f21dfcc9f846b9d. Measuring the database/WAL/SHM file set fixes the previous physical-byte reporting P2, including the held-reader case. Moving requested compaction after registration removes it from startup, but two P2 failures remain in the new path: synchronous compaction blocks the live Host event loop, and a transient database lock silently consumes the request. See inline findings.
Node 24 build:test, 29 focused storage tests, renderer architecture, ASF headers, merge-tree against current main, and diff-check passed. Biome formatting reports three changed files; hosted checks are not yet available. I did not run packaged Desktop or a multi-GB compaction.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| startMaintenance: () => storageMaintenance.start(), | ||
| startMaintenance: () => { | ||
| storageMaintenance.start(); | ||
| void runRequestedRuntimeDatabaseReclamation(workspaceRoot).catch((error) => { |
There was a problem hiding this comment.
P2: void does not move compaction off the Host's event loop. After the awaits, runRequestedRuntimeDatabaseReclamation executes synchronous DatabaseSync VACUUM/incremental-vacuum/checkpoint on this same thread, now after the Host is registered and serving connections. In a real locked-database probe, a 100 ms Node timer fired only after 5,005 ms; a large successful VACUUM can block for much longer. Please run this maintenance outside the serving event loop (with a bounded lifecycle) rather than merely starting the promise after registration, and test Host responsiveness while it runs.
| if (error instanceof RuntimeDatabaseReclamationError && error.reason === 'insufficient_disk_space') { | ||
| throw error; | ||
| } | ||
| await clearRuntimeDatabaseCompactionRequest(workspaceRoot).catch(() => undefined); |
There was a problem hiding this comment.
P2: A transient SQLITE_BUSY now clears the user's compaction request, while the caller only logs the error and the report query exposes no failure. With a second WAL connection holding BEGIN IMMEDIATE, this exact path waited ~5,008 ms, threw database is locked, and left the journal as {version:1}; releasing the lock and restarting cannot retry, and Settings sees runtimeCompactPending=false without a successful report. Keep the request for a safe retry/backoff, or persist a visible failed state until the user can act.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e7ff8df8daf84af477f75a143f21dfcc9f846b9d (incremental from fa023f71). Independent second pass; I agree with the two P2s already posted on this head and add supporting measurements rather than duplicate inline comments.
Fixed since the last round
- Reclaimed bytes are now the physical file-set delta on both maintenance lanes. With a second connection holding a read transaction, both context-offload and runtime lanes report 0 bytes (253,872 without the reader; the old page arithmetic would have claimed 262,144).
- Compaction no longer runs before the Host listens, and a failed attempt no longer reruns on every start.
P2 — compaction still blocks the live Host (agrees with the existing inline on execution-composition.ts)
The full VACUUM in sqlite-runtime-reclamation.ts is still synchronous on the Host's main thread, now while clients are connected. Holding the DB open the way a running Host does, a ~300k-row runtime DB blocked the event loop for 2.5 s and a ~1M-row (~1 GB) DB for 13.0 s. Clients ping every 2 s and drop the connection after 8 s without a reply (client/connection.ts:94-97), so large compactions would disconnect attached clients (inferred from the timeout; not observed in a full Desktop run). Data safety is fine: the DB stayed usable, integrity_check passed and rowids survived. Running compaction in a worker thread, or only while no clients are attached, would avoid this.
P3
- A failed compaction clears the request silently (the second existing P2 covers the
SQLITE_BUSYcase); insufficient disk space instead stays "pending" indefinitely. Neither is surfaced in Settings. - The deferred compaction is not tied to Host shutdown and can start while the Host is draining (SQLite locking keeps it safe).
- Still present: redundant compatible-change declaration alongside the 198→199 epoch bump; vacuum under
#readTransaction; runtime lease opened every tick; unused page-count estimate frommaintainOperationalStateDatabasePages. - No tests cover the deferred compaction or the reader-held case on the runtime lane.
Checks run locally: renderer-architecture, git diff --check, ASF headers, epoch check, Biome on changed files; storage 73/73, runtime-host 103/103, host-kernel/execution-composition 129/129 — all pass.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Reviewed exact head e7ff8df8d as the author of the tracking issue (#5825) and of M1 (#5832, merged as 0649970). Thanks @garvit-arora for taking M2 on, and for turning around four review rounds in a day. The context-offload lane has converged: the checkpoint runs after commit, periodic ticks are PASSIVE, and reclaimed bytes are the physical file-set delta. That part looks close.
I won't repeat the two open P2s from @hqhq1025 and @Astro-Han on this head (runtime compaction blocking the live Host; SQLITE_BUSY consuming the request). I agree with both. What follows is what changed now that M1 is on main, plus scope.
P1 (merge-blocking): the branch no longer merges, and the epoch it claims is taken.
mainis now at epoch 199 (#5832). This PR also moves 198 → 199, andgit merge-treeagainst currentmainconflicts inprotocol/index.ts,server/execution-composition.tsandpreload/runtime-host-renderer-operations.ts.- Fix: merge
main, move to 200, and deleteprotocol-compatible-changes/storage-reclamation-operations.json. With a bump, the declaration is redundant, and its rationale doesn't hold for a new Client talking to an older Host. We hit exactly that on #5832: an older same-epoch Host drops the connection on an unknown operation key.
P2 (scope): split runtime compaction out, and land the context-offload lane first.
The two open P2s are both in the runtime.sqlite compaction path, and they pull in opposite directions:
- before listen, it pushes Host startup toward the 75 s deadline (~55 s for a 1 GB DB, measured in round 2);
- after Ready, it blocks the serving event loop (13 s for ~1 GB, measured in round 4).
Neither placement is right without a real design choice. The options are a worker thread with its own connection, running only while no client is attached, or pre-listen with a startup progress state and an extended deadline. That was open question 1 on #5825, and no maintainer has answered it yet.
I'd suggest this PR keeps only:
- the bounded
incremental_vacuumlane plus the PASSIVE checkpoint forcontext-offload.sqlite(alreadyauto_vacuum=INCREMENTAL); storage.reclamation.report.query.
Move the conversion request, the journal and the compaction runner to a follow-up, once #5825 settles the approach. This PR then becomes low-risk and mergeable, and the follow-up can be designed and tested on its own; right now it has no tests.
P2 (UI): put this in the M1 Storage group instead of growing legacy settings/.
- The ledger. This PR raises the
renderer-architecture.jsonallowance fordata-settings-page.tsx, adding two bridge calls, oneuseEffectand oneuseState. Round 3 left that as a maintainer call. Now that M1 has landed a feature slice for this page area (renderer/features/storage-usage, with ports, services andStorageUsageSection), a new legacy allowance isn't needed. - Duplicate UI. It also avoids a second "Storage" section next to M1's: M1 already shows "Reclaimable space", and "reclaimed" and "compact" belong beside it.
- Fix: add the reclamation report to that slice's port and render it in
StorageUsageSection. Then drop the ledger bump and the parallel copy insettings-data-copy.ts.
P3
- Copy doesn't match the code. It says compaction "requires roughly the database size in free disk space", but the check requires 2× (
RUNTIME_COMPACTION_DISK_HEADROOM_FACTOR = 2,sqlite-runtime-reclamation.ts:43). If compaction moves to a follow-up, this goes with it. - PR body. Please change
Fixes #5825 (M2 portion)toPart of #5825.Fixeswill close the tracking issue on merge, while M3 and the retention steps still hang off it.
Checked:
- no path deletes user rows, and clients can't choose a path (agreeing with Astro-Han);
- the
settings/ledger delta and the conflicting files (git merge-treeagainstmain@28cc4e6); - the epoch on
main; - the headroom constant against the copy.
I didn't re-run the storage/runtime-host suites; both reviewers ran them on this head.
Net: rebase onto main at epoch 200, keep the context-offload lane and the report, render them in the M1 Storage section, and move runtime compaction to a follow-up once #5825 decides where it runs.
github-actions
Bot
added
effort/XL
|
@liugddx Thanks for the direction — pushed \ |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 095b0a5ae6908c7b78ced6c16819a00739ddccb0. This revision narrows reclamation to the context-offload incremental-vacuum lane; it removes the previous synchronous runtime-database compaction and its lost-request path. The context-offload path and report query are covered by passing local focused tests. The head is not merge-ready: the compatibility epoch collides with current main (inline finding), the current-main merge has a protocol-index conflict, and npm run format:check fails on five changed files.
Node 24 npm ci, build:test, 36 focused Storage/Host/Desktop tests, renderer architecture, and diff-check pass. No exact-head hosted checks are attached. I did not run packaged Desktop, multi-GB reclamation, or a real Host with context-offload churn.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 199 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 200 as const; | ||
| // 200: `storage.reclamation.report.query` surfaces cumulative reclaimed bytes. |
There was a problem hiding this comment.
[P1] Allocate a new compatibility epoch against current main. This head assigns epoch 200 to storage.reclamation.report.query, but current main 5ac266b1 already assigns epoch 200 to Agent Graph operator snapshots. Peers built from these two trees would pass the equality handshake while disagreeing on wire shape and available operations. A fresh merge-tree reports a conflict in this file, and node scripts/protocol-epoch-check.mjs --base 5ac266b1 rejects the unchanged epoch. Rebase and advance past the current main epoch before this protocol addition can ship.
|
Rebased onto latest \main\ again in \89402f98c. Protocol epoch is now 201 (200 is claimed by Agent Graph previews merged on main). Merge conflicts resolved. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head after its merge with main. The compatibility epoch is now 201, distinct from main’s Agent Graph epoch 200; the earlier protocol collision is resolved, and the static merge is clean. The PR remains blocked by current-head checks: test fails on five unformatted PR files (reproduced locally with npm run format:check); audit and the immutable tarball fail on the shipped dependency audit (brace-expansion@5.0.9 and fast-uri@3.1.7). The lockfile is unchanged from main, so I do not attribute those advisories to this PR, but the release gates are red. No additional substantiated P0–P3 finding in the scoped reclamation path. Node 24 build:test, 34 focused Storage/Host tests, diff-check, and static merge against main pass. I did not run packaged Desktop, multi-GB compaction, or live Host churn. Not ready to merge while the checks fail.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 89402f98c. Thanks @garvit-arora, the scope split landed as suggested:
- runtime compaction and
storage.reclamation.requestare gone; - the result shows in the M1 Storage slice instead of legacy
settings/; - the body says
Part of #5825.
Most earlier findings also hold as fixed on this head:
- the checkpoint runs after commit;
- periodic checkpoints are PASSIVE;
- with a reader holding a snapshot, 40 ticks reported 0 and the first tick after release reported the full 10.7 MB, matching the file shrink;
- normal writes don't inflate the total.
It still merges cleanly with main@5ac266b, and the two new test files pass locally.
P2 (scope): the lane is the change; the report pipeline is now most of the diff.
With one lane left, the rest of the diff exists to show one cumulative number:
- a journal file with fsync, temp-file rename and a directory fsync;
- a new operation with its coordinator, preload bridge, IPC case, renderer allowlist entry and remote grant;
- an epoch bump;
- a UI row.
That number costs a journal write on every productive tick: 259 writes, each with 3 fsyncs, to drain 64 MB of free pages here. It is also hard to act on:
- it is a lifetime lower bound, and it doesn't survive a new root;
- it sits next to M1's "Reclaimable space", which measures a different database (
runtime.sqlite), so it reads as "Y of X reclaimed".
Once the lane runs, context-offload.sqlite shrinks, and that already shows in M1's context_offload total.
My suggestion is to land just the maintenance lane: no protocol change, no epoch, no journal, no UI. That is the smallest version that frees disk. If a visible figure is still wanted later, the honest shape is an in-memory "freed since Host start" (or "last maintenance freed X at T") as an optional field on storage.usage.query, not a second operation.
This also removes the epoch race: this head and #5884 (mine) both claim 201, so whichever lands second would have to re-pin.
P2: incremental_vacuum still runs in a deferred (read) transaction. sqlite-context-offload-store.ts:525 uses #readTransaction, and PRAGMA incremental_vacuum writes.
- A deferred
BEGINupgrades mid-transaction. Under a competing writer that isSQLITE_BUSY_SNAPSHOTwith no wait, or up to the 5 s busy timeout on the event loop. - The sibling
collectGarbagelane uses#writeTransaction(BEGIN IMMEDIATE). - This is plausible rather than reproduced, since the store has one writer connection per process.
- Fix: use
#writeTransaction, and rename the test that currently pins "read transaction" (sqlite-context-offload-store.test.ts:1092).
P2: the tests don't exercise the lane (confirmed by mutation).
- Store test:
sqlite-context-offload-store.test.ts:1113assertsreclaimedPages >= 0, and the fixture has no free pages. With the vacuum stubbed to return 0 and the checkpoint removed, it still passes. - Maintenance test:
storage-maintenance.test.tsonly stubs a lane that returns 0. With the new lane made a no-op and theonReclaimedcall removed, all 4 tests still pass. - Please add:
- a store test that creates real free pages (e.g. delete many inline rows), then asserts
reclaimedPages > 0, that the file shrinks, and that the freelist drops; - a held-reader case (reports 0 while the reader is held, catches up after release);
- a maintenance test where the lane returns bytes > 0 and the result is observed.
- a store test that creates real free pages (e.g. delete many inline rows), then asserts
If the report stays (only if you keep the journal):
storage-reclamation-journal.ts:48validates onlyversion. A structurally wrong but valid JSON ("contextOffloadReclaimedBytes":"12") turns+10into string concatenation ("1210", then"121010"). The query then fails its own decoder on every call, and the row never shows. Validate each count withisSafeInteger && >= 0, and treat anything else as corrupt.- Write failures (for example on a read-only disk) log on every tick with no backoff, and they leave the
.tmpfile behind. reclamationReportisn't scoped byhostKey(storage-usage-section.tsx:80), so after a Host switch it briefly shows the previous Host's number.
P3
- Leftovers from the removed compaction path.
runtimeReclaimedByteshas no writer and is always 0, but it sits in the protocol, the journal and the renderer sum.runExclusiveWalCheckpointandreadSqliteReclaimableByteshave no callers. The./sqlite-page-reclamationpackage export has no cross-package consumer. - Second copy of the file-set helper.
readSqliteDatabaseFileSetBytes(sqlite-page-reclamation.ts:86) duplicates M1's file-set measurement, and it misses-journal, whichsqlite-file-set.tsincludes. Reuse that helper. - Wasted checkpoints. When the freelist is empty, the lane still runs a checkpoint every 60 s. Skip it.
- Formatting.
npm run format:checkfails on five files:storage-reclamation-protocol.test.ts,execution-composition.ts,sqlite-context-offload-store.ts,sqlite-page-reclamation.tsandstorage-reclamation-journal.ts. That is the redtestjob.
CI, not yours: audit and Build immutable tarball fail on new advisories for brace-expansion (high) and fast-uri (moderate). The same audit fails on main, and this PR doesn't touch package-lock.json.
Checked:
npm ciandbuild:test;- the PR's new and changed tests, plus 201 runtime-host protocol and allowlist tests and 100 desktop storage/bridge tests, all passing;
- the held-reader and drain measurements above; each tick is bounded to 64 pages (vacuum p99 13 ms, full tick max ~220 ms including async stat);
- two dist mutations, both restored;
- a merge-tree against current
main.
Net: keep the lane, move it to #writeTransaction, give it real tests, and ship it without the journal, operation and epoch. A visible "freed" figure can follow as an optional storage.usage.query field if we decide we want one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
This revision formats five existing reclamation files; it does not change the reclamation behavior. The previous formatting failure is fixed: format:check now passes. The context-offload vacuum lane and report pipeline remain as reviewed on the preceding head. I did not find a new issue introduced by this revision.
The substantive concerns in the existing review by liugddx remain at the same code paths. In particular, reclaimFreePages still executes the writing incremental_vacuum pragma inside a deferred BEGIN (sqlite-context-offload-store.ts:528 and :1289), and the journal parser still validates only version before using persisted counts in arithmetic (storage-reclamation-journal.ts:45-49,110-113). I verified these control paths, but did not reproduce the competing-writer timing or a corrupt-journal recovery run. I am not duplicating the earlier review comments.
For this exact head, Node 24 build:test, format:check, and 34 focused Storage/Host tests pass. The protocol epoch advances from current main 200 to 201; the epoch guard, diff check, and merge-tree against main@d7dffca9 pass. Hosted audit and immutable-tarball checks fail on shipped brace-expansion and fast-uri advisories; the PR does not change the lockfile. Hosted test is still running at review time. This is not a merge-ready result. I did not exercise live Host churn, packaged Desktop, or native Windows/macOS.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Ship only the bounded incremental_vacuum maintenance path per maintainer feedback: no protocol/epoch/journal/UI. Reclaim free SQLite pages via write transactions and a passive WAL checkpoint, wired into HostStorageMaintenance. Generated-by: Cursor
25feafb to
c36d591
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit c36d591a97d4feae87f936724cd3327c3a57a140. This branch is now limited to a bounded context-offload SQLite page-reclamation lane. HostStorageMaintenance schedules at most 64 pages per batch after startup (packages/runtime-host/src/server/storage-maintenance.ts:24-27,69-94); the writer facade serializes the call (packages/storage/src/context-offload-store.ts:243-249); the store skips an empty freelist, runs the vacuum in an immediate write transaction, and then attempts a passive WAL checkpoint (packages/storage/src/sqlite-context-offload-store.ts:519-547,1286-1297). The previous runtime-database compaction, journal, report query, and protocol-epoch changes are absent from this head. I found no substantiated P0-P3 issue in the inspected lane.
Node 24 full build:test and 31 focused Storage/Host tests pass locally. Fetched main c838e1fa merges cleanly, and git diff --check passes. This head has no hosted checks, and local npm run format:check fails at packages/storage/src/sqlite-context-offload-store.ts:527 (line wrapping). It is not merge-ready until that gate is repaired and current-head CI runs. I did not exercise a held SQLite reader, a large live Host database, native platforms, or the full local test suite.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
github-actions
Bot
added
effort/M
Generated-by: Cursor
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 57ab85aee9e1e7ebfa82cffe3f62356611199e5f. Since c36d591a the only new commit is a whitespace-only Biome reformat of packages/storage/src/sqlite-context-offload-store.ts:527-529. It wraps the over-long #readTransaction(...) call, which was the one blocker in the previous review. The code is otherwise unchanged from the head that was reviewed clean. The branch still merges cleanly with current main, and the hosted test job, which includes the format check that failed before, now passes.
No P0-P3 findings. I did not re-run the storage suites locally for this whitespace-only increment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
This branch has not been deployed
4 participants
Summary
incremental_vacuumfor context-offload SQLite via the existing maintenance loopstorage.reclamation.report.query(protocol epoch 200) to surface cumulative reclaimed bytesScope split (per @liugddx review): runtime
sqlitecompaction andstorage.reclamation.requestare deferred to a follow-up PR so this lands the context-offload lane only.Part of #5825. M1 storage visibility is tracked separately in #5832.
Test plan
packages/storageunit tests for reclamation journalpackages/runtime-hoststorage maintenance and protocol decode testsapps/desktoprenderer architecture check