fix(startup): simplify tool validation and fix font loading - #5614
Conversation
Rebuild tool validation from the current transaction's local dependency closure, removing the retained reducer cache and its consistency protocol. Remove recovery profiling wrappers and keep fonts as local CSP-safe assets. Add mixed tool/steering restart checks and a reusable correctness runner. Refs apache#5613 Generated-by: OpenAI Codex
Keep the startup correctness file inventory visible to the release contract checker. Remove dynamic path construction without changing any selected test or group. Generated-by: OpenAI Codex
Keep the self-contained correctness runner and generated-data regression tests in the PR. Retain the handoff document only as local validation material. Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the current head c106ed8d8b0e7c95403e8205eeb043ffecff7f39. I found no substantiated P0–P3 issue in the changed paths.
The change removes the cross-transaction tool-ledger reducer cache and validates each proposed append against dependency events read in the current SQLite transaction (packages/storage/src/sqlite-runtime-store.ts:4263, packages/core/src/tool-ledger-scanner.ts:228). It also removes startup profiling wrappers and keeps KaTeX font files external so the renderer CSP can load them (apps/desktop/vite.config.ts:65). I checked transaction/rollback behavior, existing-versus-candidate corruption handling, recovery flow, packaging, and the adjacent tests. There is no database schema migration in this PR.
The current-head test, audit, Linux packaging, packaging, Windows owner, and macOS owner checks pass. The branch merges cleanly with current main. I could not rerun tests locally (this checkout lacks dependencies and the host has Node 18), measure production-scale startup/write performance after cache removal, or visually verify font loading. This is not a merge approval.
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.
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up to my earlier review of the same head (c106ed8d8b0e7c95403e8205eeb043ffecff7f39):
[P3] Removing the reducer cache makes each tool-bearing append re-read and decode every existing RuntimeEvent in the candidate's invocation, not just the candidate's direct dependencies. packages/storage/src/sqlite-runtime-store.ts:4283-4317 selects all rows with WHERE invocation_id = ? and can expand to parent-operation invocations; packages/core/src/tool-ledger-scanner.ts:233-260 rebuilds and scans the resulting set. For a single long invocation with k accumulated events, one new append therefore requires at least O(k) validation work, and successive appends can total O(k²). This does not grow with unrelated session invocations: the operation lookup has an expression index (packages/storage/src/sqlite-runtime-schema.ts:699-706). The new 100k-history test primarily covers unrelated history, not repeated writes to one long invocation. Please add a representative long-invocation write regression or measurement and consider a bounded way to avoid repeated full-prefix decoding. I have not measured user-visible latency, so I classify this as P3 rather than a demonstrated higher-severity slowdown.
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.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
The PR removes the cross-transaction tool-ledger reducer cache (LRU budgets, checkpoint/undo log, write-view invalidation, settlement synchronization) from SqliteRuntimeStore/@maka/core and replaces it with per-write validation rebuilt inside the current SQLite transaction, scoped to the candidate invocation plus its explicit parent-operation closure; it also drops the MAKA_STARTUP_PROFILE layer in SessionManager and keeps KaTeX fonts as files to satisfy the renderer CSP. The premise is real on all three fronts: the deleted machinery existed in base and carried heavy consistency obligations, and the CSP at apps/desktop/src/renderer/index.html:24-27 (default-src 'self', no font-src, no data:) does block Vite-inlined data-URL fonts. The direction is right — validation semantics (validateToolLedgerTransition, packages/core/src/tool-ledger-scanner.ts:228) are byte-for-byte the old incremental logic minus statefulness, and the dependency-closure BFS (packages/storage/src/sqlite-runtime-store.ts:4283) matches the deleted cache-miss path; SQLite read-your-writes covers in-transaction retries since inserts immediately follow validation. The disclosed per-write rescan cost is an honest, documented trade-off.
Findings
- [P3] scripts/test-startup-correctness.mjs:30 — the runner's default output
artifacts/startup-correctness/latestis not covered by.gitignore(only*.logis ignored);report.jsonandworking-tree.patchbecome untracked files after every documented run, contradicting the PR body's "generated reports are excluded". Addartifacts/startup-correctness/to.gitignore. - [P3] packages/runtime-host/src/tests/startup-state-matrix.test.ts:52 — the flagship 100k-background-event variant runs only via the manual script; no CI workflow references
test:startup-correctness(repo-wide grep hits onlypackage.json:35), so CI (testjob,runtime-hosttest:dist) enforces only the 1k default. The headline scale journey can rot silently; consider a scheduled 100k lane or soften the claim. - [P3] packages/storage/src/sqlite-runtime-store.ts:4263 — the worst case the cache absorbed (many tool writes inside one long invocation, now O(history) per write, O(N²) per invocation) is exactly what the new matrix does not exercise:
historyEventsare spread acrossMath.min(300, …)invocations (~333 events each at 100k) and tool targets sit in their own invocations. Disclosed in the body with measurements, so acceptable — but the suite cannot detect a future blowup of that shape.
Verdict
merge-ready — premise real, simplification correct and well-tested; only three minor follow-ups (gitignore, CI wiring for 100k, unstressed worst case).
|
@hqhq1025 @Astro-Han @me2seeks 已复核并更新到当前 head
当前 head 本地验证:Core/Storage/Runtime/Runtime Host/UI 依赖构建通过;startup matrix 2/2;Vite contracts 2/2;Biome、 |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head, c1ddf366, including the upstream-main integration and the two follow-up commits. The latter adjust the startup recovery matrix to the current unknown-outcome contract, exclude local review artifacts, and replace an immediate Windows sandbox-process baseline check with the existing bounded observation helper. I found no new substantiated P0–P2 issue.
[P3] The single-invocation append cost reported on the previous head remains in the current code. Each tool-bearing admission calls readToolLedgerDependencies (packages/storage/src/sqlite-runtime-store.ts:4360-4417), which loads every stored event for the candidate invocation and connected parent invocations. validateToolLedgerTransition (packages/core/src/tool-ledger-scanner.ts:228-260) then reconstructs and scans that history. For an uninterrupted invocation with k prior tool events, the next append does O(k) validation work; k sequential appends can therefore do O(k²) cumulative work. This does not imply a scan of unrelated sessions. The existing large-background tests do not measure a single long invocation's incremental append latency. A focused thousands-of-appends benchmark would make the accepted trade-off measurable; I have not measured latency on this head.
Validation: the current-head hosted test, audit, Linux/package, and Windows/macOS owner checks pass; the changed Windows script passes Node 24 syntax checking, git diff --check and merge-tree against current main are clean. I did not independently run the complete startup matrix or a packaged Windows sandbox smoke test. No schema migration is added by the follow-up commits.
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.
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head c1ddf366 (a different model lineage from the parallel review). No P0–P2 found.
The "simplification" removes the cross-transaction reducer cache added in #5556. The reducer's interpretation rules are unchanged apart from dropping the cache's write wrappers. In detail:
- Checks unchanged: dispatch-before-result and the orphan/order/identity/spine/duplicate checks are intact (
tool-ledger-scanner.ts:314-545). - Parent/child linkage unchanged: its load scope matches the old cache-miss path (
sqlite-runtime-store.ts:4378-4425). - Recovery unaffected: the unsettled-operation terminal guard is SQL over
tool_operationsand unaffected (:4625-4642). #5770'sensureRecoveredTerminalRuntimeEventDurabledoesn't go through ledger validation (:469-556). - Consistency is stronger: validation now always re-reads inside the transaction instead of relying on
writeViewRevisionanddata_versionfor cache invalidation. - No leftovers: the removed exports have no remaining users, and there is no schema change.
The font fix is correct. The renderer CSP has no font-src, so data: fonts are blocked. Size3-Regular.woff2 (3624 B) is the only KaTeX font below the 4096 B inline threshold, and a partial Vite build confirms it is now emitted as a file.
P3 (non-blocking):
-
Per-write re-read cost (performance). Each tool write re-reads the whole invocation and rebuilds the reducer (
sqlite-runtime-store.ts:4359-4425). CPU-time probes, order-of-magnitude only on a shared machine:Scenario This PR main 1000 back-to-back tool commits, 512 B results 54.3 s (~70 ms/tool at the tail) 12.5 s (~20 ms/tool) 500 tools with a streaming partial before each 15.5 s 16.6 s 1000 tools, 4 KiB results 55.7 s 43.6 s (main's cache degrades after ~600 tools) So the quadratic cost already exists on main in realistic streaming loops, and this PR only removes the optimisation for the narrow back-to-back case. It isn't on the startup path. A follow-up could select only tool-relevant events in SQL.
-
A test assertion is too weak. In
startup-state-matrix.test.ts:129the three prepared scenarios only assert "nofunction_response". They don't assert thattool_operationsendedinterrupted_unknownor that the unsettled list is empty, which is what #5770 guarantees. -
The tests don't distinguish old code. The PR's storage tests (68/68) and matrix tests (2/2) also pass on the main baseline. That's expected for a behaviour-preserving removal, but it means "tests fail on the old code" doesn't hold for these. The font fix has no regression test; a build-contract check that CSS contains no
data:fontwould pin it.
Verified:
- core/storage touched tests 135/135;
startup-state-matrix(1k history) 2/2;- a baseline comparison run.
Not verified: Node 24 (Node 22 was used), the 100k matrix, Electron KaTeX rendering, and Windows sandbox e2e.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 49ac36db04d100af82c57eca800d8507c8e8f16e. The new commit adds two contract checks: startup-state-matrix.test.ts:134-152 reads persisted tool-operation state after recovery and requires no unsettled operations, while vite-client-plugin-slots.test.mjs:60-89 builds KaTeX CSS using the renderer's asset-inline setting and requires emitted font files rather than data-font URLs. These target the startup recovery and CSP/font behaviors changed by this PR. I found no substantiated new P0–P3 issue in the follow-up or main merge; the previous findings in my area remain closed.
The current-head hosted test, audit, Linux/package, and Windows/macOS owner checks pass. Script syntax, diff-check, and a merge-tree against current main pass locally. I could not run the new Vite test locally because this checkout has no vite installation; I also did not run the full recovery matrix or packaged Windows/macOS smoke locally.
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.
|
@hqhq1025 @Astro-Han @me2seeks Thank you for the reviews. Follow-up on current head
The branch merges cleanly with current main. Exact-head CI, Linux/Windows packaging, owner-platform checks, and dependency audit all pass. There are no unresolved review threads. |
me2seeks
left a comment
There was a problem hiding this comment.
Approve after full-diff review of head 49ac36d.
The simplification removes the cross-transaction tool-ledger reducer cache introduced in #5556 and returns to per-write validation rebuilt from the current SQLite transaction, while keeping #5556's scoped-read semantics. I verified the pre-#5556 logic against this head: validation rules, idempotent dedupe, and CorruptionError/RejectionError classification are preserved; the two deliberate deltas (dependency-closure failure domain instead of workspace-wide fail-stop; reads widened to the explicit parent-operation closure) are sound — validation scope now exactly equals failure domain, and global integrity is correctly separated from local write validation.
The disclosed single-long-invocation O(k²) trade-off is accepted; it remains cheaper than the pre-#5556 workspace-scan-per-write model. CI is green on this exact head; no unresolved threads. Non-blocking transparency nits: the KaTeX font fix is a separate domain (though declared in the title), and the Windows sandbox e2e baseline-wait change is not mentioned in the PR body — worth declaring incidental fixes in the description in the future.
Summary
Fixes #5613. Follow-up to #5556, based on current main.
Tool writes now reconstruct validation state from the current SQLite transaction, scoped to the invocation and explicit parent dependencies. Remove the cross-transaction reducer cache and its LRU/budgets, checkpoint/undo, invalidation versions and settlement synchronization. Scans and writes retain shared validation rules, corruption classification and exact retry semantics. Also remove the leftover
MAKA_STARTUP_PROFILEtiming layer from SessionManager recovery.The trade-off is deliberate: the previous same-base ablation retained the startup improvement, while a synthetic single invocation with 200 uninterrupted tool executions was slower without caching (615→1249 ms for 512 B; 839→2056 ms for 4 KiB). Short invocations and heavily interleaved partial writes showed smaller or unstable benefits; some intermediate patterns still benefit. We accept that local cost to remove long-lived consistency obligations whose representative end-to-end value has not been established. These are prior measurements on
518fd529c0, not new performance claims for this PR's base.Keep bundled fonts as local assets so KaTeX history rendering respects the existing CSP. The memory/model-connection startup readiness fix is already supplied by #5606 on main.
Add two real Host/SQLite regression journeys: mixed prepared/committed/unknown tool outcomes recover once; consumed quoted steering survives SIGKILL without duplicate echo or old-epoch replay. Run both with 1k and 100k background events, checking history digests and repeated recovery.
npm run test:startup-correctnessgroups these with existing storage, tool, queue, background-task and Desktop contracts. Local benchmark drivers, databases and generated reports are excluded.Verification
49ac36db0contains review-test commit09ec6f699and merges current main49dacdf13; GitHub reports the branch mergeable.interrupted_unknownand that no unsettled tool operation remains. It continues to assert that no syntheticfunction_responseis invented.data:fontURL, and requires emitted font assets. It fails under Vite's default 4096-byte inlining policy.git diff --check, and the protocol epoch guard pass.preparedfails against the actualinterrupted_unknownstate.AI use
Tool(s) and scope: OpenAI Codex implemented the simplification and font fix, authored the mixed-state tests and runner, ran validation, and prepared this submission at @testikun's request. @testikun is the human contributor of record; final review and merge remain with humans.
Checklist
Does this PR entail a change in behavior?