fix(storage): load node:sqlite through one lazy, quiet seam - #1
Closed
childrentime wants to merge 1 commit into
Closed
childrentime wants to merge 1 commit into
childrentime wants to merge 1 commit into
Conversation
Every CLI invocation printed Node's SQLite ExperimentalWarning to stderr, including `maka --help`, which never opens a database. Two causes, both regressions against apache#1257: - Three modules reachable from the `@maka/storage` barrel took a static value import of `node:sqlite` (`operational-state-backup`, `session-bundle-policy`, `operational-target-schema`). Node evaluates a builtin the moment it enters the module graph, so importing the barrel paid for SQLite unconditionally. - The warning suppression that makes an actual database open quiet existed in two copies (`operational-state-store`, `sqlite-session-metadata-store`) and was missing from the other three load sites, so whichever module loaded first still leaked the warning. Route every load through `sqlite-module.ts`, which is lazy, memoized, and silences that one warning for exactly the duration of the load. Deprecation and all other warnings keep their normal path. Restore `package-import.test.ts`, removed in apache#2710, and widen it: it now also asserts that constructing a store is quiet and that no compiled module carries a static `node:sqlite` specifier. `packages/eval/src/maka-artifacts.ts` keeps its static import; @maka/eval does not depend on @maka/storage and is not on the CLI startup path. Generated-by: Claude Code
Owner
Author
|
方案作废:改用彻底删除 barrel 的做法,见新 PR。惰性加载 + emitWarning 猴子补丁属于过度设计,被 review 否掉了。 |
childrentime
pushed a commit
that referenced
this pull request
Aug 24, 2026
…ons (apache#2600) (apache#3001) * feat(runtime): surface worker disconnect as unknown outcome for mutations (apache#2600) A filesystem mutation whose worker fails after dispatch may already have landed on disk, but the host reported it as a generic error. The model had no way to tell "the write may have happened" from "nothing ran", so it treated a half-applied mutation as a clean failure. Close that gap for the disconnect concern: - process-runner tracks a `dispatched` flag (set once Node's 'spawn' event fires and stdin is written) and surfaces it on both the resolved result and the rejection error. - client splits the ambiguous launch failures by that flag: `spawn_failed` (never started, nothing could have been written) vs new `worker_io_incomplete` (ran but the result was lost). `aborted` keeps its reason but carries `dispatched` so a pre-flight cancel is distinct from a post-dispatch kill. Worker-response failures are marked dispatched. - protocol gains an `outcome_unknown` error code so the worker can report "I may have applied this before I lost the ability to answer". - the boundary executor converts a mutating op that fails with a post-dispatch reason into ToolOutcomeUnknownError, which already flows to a structured uncertainOutcome result. Reads and pre-flight failures pass through unchanged. This is the first of four commits addressing apache#2600; it independently closes the "post-dispatch unknown outcomes" concern. Generated-by: ZCode * refactor(runtime): extract filesystem-authority contract (apache#2600) Issue apache#2600 asks for the filesystem-authority contract to live in one place, separate from the individual editing tools. Introduce that module as pure types and a classifier, with no I/O. - FilesystemTargetIdentity: opaque decimal-string dev/ino. String rather than bigint because bigint cannot cross the worker's JSON protocol boundary, and identity is only compared for equality, never used to build a path. - FilesystemTargetDescriptor: a discriminated union so the "no identity to compare" case is an explicit `missing` arm, never an accidentally-absent optional field. A future "skip the identity check" change cannot compile without handling `missing`, which closes the "no identity -> CAS passes" regression class. - FilesystemMutationOutcome: applied | rejected | unknown. - classifyFailedMutationOutcome + UNKNOWN_OUTCOME_REASONS: moved out of filesystem-executor.ts so the executor consumes the contract instead of restating it. The classifier's invariant (every member reason is semantically dispatched, so membership alone suffices; only `aborted` straddles pre-flight/post-dispatch and gates on the flag) is documented at the contract. This is the second of four commits for apache#2600. It is types plus a behaviour- neutral refactor; the descriptor and identity are wired into the worker protocol and the fd-pinned read-modify-write in the following commit. Adds filesystem-authority-contract.test.ts covering the classifier branches and the type-level constraints. Generated-by: ZCode * feat(runtime): capture target identity at lock acquisition for CAS (apache#2600) Close the queue window for issue apache#2600 concern #1: a path replaced while a mutation waits for the write lock must be detected, not silently written. The identity is now captured at lock acquisition (T0) — before the call enters the lock queue — not re-derived inside client.execute after the lock is held (T1). Re-deriving at T1 would sample the post-replacement inode, making the CAS self-fulfilling and re-opening the window. - protocol v6: FilesystemWorkerTargetSchema gains an optional identity {dev, ino} (opaque decimal strings; bigint cannot cross the JSON boundary). superRefine rejects a missing target carrying an identity. - filesystem-executor: writeLockTarget returns {key, canonicalPath}; captureIdentityAtLockAcquisition stats the canonical path at T0; run() receives expectedIdentity and passes it to worker.execute; the FilesystemWorkerExecuteInput carries an expectedIdentity field. - client: deleted captureTargetIdentity (the T1 capture); client.execute uses the caller-supplied expectedIdentity verbatim. - worker assertTargetUnchanged compares the on-disk inode against the T0 identity (follow/entry stat modes match the targetType derivation); a non-missing WRITE target must carry an identity or the request is rejected (reads are exempt — they do not mutate). - post-write orphan check: assertPathStillMatchesIdentity re-stats the path after a write and reports outcome_unknown if the inode no longer matches (the write went to an orphaned inode; the visible file is the replacement). Red-line test verifies the worker receives the T0 identity (before a replacement), proving the capture happens at lock acquisition. Direct unit tests cover the post-write orphan check (match / replaced / disappeared). This is the third of four commits for apache#2600. Generated-by: ZCode * feat(runtime): extend post-write orphan check to edit/format/update (apache#2600) Commit 3 added the post-write identity check to the write operation only. Extend it symmetrically to edit, format_json, and apply_patch update so a path swapped during any read-modify-write is reported as outcome_unknown, not a misleading success. The delete path is already covered by the T0 identity CAS in assertTargetUnchanged (it runs for every operation, uses lstat for the directory-entry semantics that create/delete use, and rejects with path_changed when the inode mismatches). format_json's invalid-JSON branch already returns ok:false without writing. No additional changes needed for either. Adds red-line tests proving a delete and an edit whose target was replaced after authorisation are rejected (path_changed) and the replacement file is left untouched. This is the fourth and final commit for apache#2600. Generated-by: ZCode * test(runtime): pass expected identity in Linux filesystem worker smoke (apache#2600) The T0 identity CAS (b51c6a5) made the worker refuse a write mutation on an existing target that carries no identity. The macOS smoke test was updated to pass one, but the Linux smoke test's Edit call was missed — it runs only on linux+bwrap, so local runs never execute it and only CI's Ubuntu runner hit the refusal. Generated-by: ZCode * test(runtime): make the T0 and missing-target tests protect observable behavior (apache#2600) Two test-quality fixes from review: - The T0 identity test replaced the path inside the worker, i.e. after the lock had been granted — a regression that captured the identity at T1 (post-lock) would still pass, because the replacement happened after any T1 capture point. Rework it to exercise the real queue window: a first mutation blocks inside the worker while holding the path's write lock, a second mutation queues behind it, the path is replaced while the second waits, then the gate releases. Assert the second worker call receives the pre-replacement dev/ino. A T1 capture now samples the replacement's inode and fails the assertion. - The missing-target create test used an empty patch and only asserted the error was not path_changed — it accepted invalid_request, which is exactly what an over-broad mandatory-identity check would throw for missing write targets. Use a valid create patch, assert success, and assert the created file content, so that regression fails loudly. Generated-by: ZCode * feat(runtime): fd-pinned mutation primitive for both backends (apache#2600) The previous T0-identity CAS validated the target and then re-opened the pathname, so a swap between validation and the open could still divert the write onto the replacement — detection after the fact, not prevention. The local/workspace path additionally bypassed the identity authority entirely. Introduce file-stable-write.ts, the fd-pinned mutation primitive both backends now consume, and reconcile the cooperative missing↔existing transitions that previously surfaced as invalid_request: - openStableTarget: open the approved object once — 'r+' with O_NOFOLLOW for existing targets (identity validated by fstat on the descriptor, BEFORE any truncation, so a rejected validation leaves the file intact; write-only targets fall back to O_WRONLY), 'wx' for approved-missing targets (a file that appeared in the gap is path_changed, never truncated). writeThroughHandle truncates and writes at position 0 through the pinned descriptor; ENOSPC/EIO/EDQUOT/EFBIG surface as outcome_unknown — a half-written file is not a clean failure (apache#2600 review P2-2). hostVisibilityAfterWrite describes whether the path still resolves to the pinned inode after the write. - worker: write/edit/format_json/apply_patch-update run read/transform/ write through the pinned handle; format_json's invalid-JSON branch still returns ok:false without writing. - local: LocalWorkspaceExecutor gains readModifyWrite (optional on the workspace interface; remote/isolated workspaces keep the path-based fallback, documented as unprotected). Identity capture at lock acquisition no longer depends on a worker being wired (apache#2600 review P1-2), and apply_patch update resolves the existing-target requirement before any create runs. - client: the missing↔existing transitions while queued are reconciled against the T1 reality — a stale identity on a vanished target is dropped so "delete then rewrite" stays a clean apply, and a write whose target appeared while queued fails with a meaningful path_changed, never invalid_request (apache#2600 review P2-1). The duplicated mutation catch blocks collapse into settleMutationFailure, which also maps the primitive's StableWriteFailure codes. - tests: deterministic race for the pin (swap between validation and the write; bytes land on the original inode, replacement untouched), wx gap-creation rejection, both client transition directions, and the lock-serialisation causal barrier moved onto readModifyWrite. The tautological type-level contract tests are removed. Generated-by: ZCode * feat(runtime): compare-and-delete with tombstone verification (apache#2600) The delete path could remove a replacement: the entry identity was checked and then fs.unlink(path) ran against the pathname, so a rename between the check and the unlink deleted the replacement while the operation reported { ok: true } — a silent unauthorized deletion with no post-operation validation (apache#2600 review: "delete can remove replacement"). POSIX has no atomic compare-and-unlink, so prevention by unlink is impossible; make the capture atomic instead. compareAndDeleteEntry renames the entry to a private unpredictable tombstone in the same directory, verifies the tombstone carries the approved identity (lstat — works for regular files and symlinks alike), and only then unlinks the tombstone. A mismatch means a replacement was installed in the window: it is renamed back to the path — restored, not deleted — and the operation reports path_changed with a message saying so. If the final tombstone unlink fails after a verified match, the entry is preserved at the tombstone and the failure is reported as outcome_unknown, never as success. rename(2) moves the directory entry itself and needs no permission on the file (only write+execute on the parent, exactly like unlink), so deleting read-only/write-only files keeps working. Wired into both backends: the worker's apply_patch delete, and the local executor's applyPatch delete with the approved identity threaded through WorkspaceApplyPatchInput. Tests: plain removal leaves no tombstone; a replacement installed after the check is restored byte-for-byte and reported path_changed; a symlink entry is deleted by its own identity without following the link. Generated-by: ZCode * fix(runtime): no-replace tombstone restore and directory rejection (apache#2600) Two delete-path fixes from review of the compare-and-delete primitive: - The restore could destroy a concurrent newcomer: after the tombstone captured replacement C, another process creating B at the original path meant the rename-based restore atomically overwrote B — the exact class of replacement loss this module exists to prevent. Node exposes no RENAME_NOREPLACE, but link() is natively no-replace: the restore now links the tombstone back and drops the tombstone name, guarding the platform wrinkle where link() follows a symlink source by verifying the restored inode against the tombstone before dropping it. On EEXIST — or any link failure — the tombstone is preserved and the failure reported as outcome_unknown with the location, so nothing is ever lost. - A directory entry was moved into the tombstone and then stranded there: renaming a directory succeeds, but the tombstone unlink cannot (EISDIR/EPERM), so the directory vanished from its path and hid under a stray name where the previous plain unlink simply failed. Directories are now rejected up front, before anything is moved. Also fixes the exact-head Windows sandbox smoke: its pre-existing-file write now supplies the identity captured at lock acquisition (lstat + { dev, ino }), as the boundary executor does in production. Tests: a reoccupied path preserves both the newcomer and the captured entry; a free path restores the captured entry without a tombstone leak; a directory is rejected untouched. Generated-by: ZCode * fix(runtime): type-aware tombstone restore and structured directory refusal (apache#2600) Three delete-path fixes from review of the no-replace restore: - Restore the captured entry itself, not whatever it points at: link() dereferences a symlink source on darwin (implementation-defined per POSIX; verified directly on darwin), so a link-based restore of a captured symlink would plant a regular-file alias of the TARGET at the path — a foreign entry later reads/writes silently edit, while the original link stays hidden on the tombstone. The restore is now type-aware: symlinks are recreated with symlink(readlink(...)) — natively no-replace (EEXIST), round-trips the target string exactly — and only regular files use link(), which cannot misbehave for them. The inode guard stays deliberately hands-off on mismatch: after a concurrent rename-over, the entry at the path may be a foreign one, and unlinking it would destroy third-party data — the exact loss class this module prevents. - Make the directory refusal survive the race this module is about: the lstat pre-check is not an enforcement point, so a directory swapped in between the check and the capture was moved onto the tombstone and stranded there (link() cannot restore a directory; EPERM). Enforcement now lives after the atomic capture: the captured entry's type is checked first, and a directory is renamed straight back before any identity comparison. The post-capture logic is extracted as deleteCapturedTombstone for the forced-directory regression. - Throw the failure type callers understand: the bare directory-refusal Error fell through normalizeOperationError to a generic filesystem_error, so the model never learned what was refused. The refusal is now a StableWriteFailure with its own is_directory code, added to the worker protocol's error enum so the message survives classification on both backends. Tests: a swapped-in symlink is restored as a symlink pointing at its own target — the path is never left holding a foreign regular file; a directory forced onto the tombstone is renamed back, not stranded; the worker surfaces is_directory with the refusal message end to end. Generated-by: ZCode
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
一句话
maka --help从不打开数据库,却每次都往 stderr 打一行 Node 的 SQLite 实验特性警告。这个 PR 把node:sqlite的加载收敛成唯一入口,顺手补回被删掉的守门测试。现象
--version、run、TUI 启动,每一条路径都会打。这不是新问题——apache#1257 报过,apache#1258 修过,后来又坏回去了。为什么会坏
有两个独立的原因,只修一个警告还在。
原因一:静态导入又回到 barrel 里了
Node 的规则是:builtin 模块一旦进入模块图就立刻求值。所以下面这一行本身就等于"加载 SQLite",跟你有没有真的建数据库无关:
三个文件是这么写的,而它们都能被
@maka/storage的 barrel(index.ts的export *)触达:operational-state-backup.tssession-bundle-policy.tsoperational-target-schema.ts而
packages/cli/src/cli-core.ts在启动时静态导入了这个 barrel(为了拿resolveMakaDataRoots)。链条就闭合了:启动 CLI → 导入 barrel → 求值 node:sqlite → 打警告,哪怕这次执行的是--help。注意区分:仓库里还有几十处
import type { DatabaseSync },那些是类型导入,编译后完全消失,一点问题没有。只有不带type的值导入才会触发。原因二:抑制逻辑只覆盖了五分之二
光惰性加载治不了真实会话——
maka run是真的要打开runtime.sqlite的,那一刻警告照样会打。项目里其实已经有对策:临时把process.emitWarning换成一个过滤版,加载完再换回来。问题是这段十几行的代码只在两个文件里有,而且是一字不差的两份拷贝:
operational-state-store.tssqlite-session-metadata-store.tssqlite-runtime-store.ts(只惰性,不抑制)sqlite-long-term-memory-store.ts(只惰性,不抑制)managed-dependency-environment.ts(内联 require)foreign-session-store.ts(await import)codex-session-adapter.ts(await import)谁先加载谁漏警告,等于抑制形同虚设。
改了什么
1. 新增
packages/storage/src/sqlite-module.ts(48 行)整个包加载
node:sqlite的唯一入口,三件事:createRequire在函数体里require,而不是模块顶层import,所以只有真要用时才求值。process.emitWarning,只吞掉ExperimentalWarning且正文以SQLite is an experimental feature开头的那一条。用try/finally保证一定换回来。Deprecation 和其它任何警告完全不受影响。cached变量存住结果,避免重复的emitWarning换进换出。(require自己有模块缓存,这层是省开销,不是保证正确性——你觉得多余可以砍掉。)对外导出两个函数:
loadSqliteModule()拿整个模块(有的调用点要用里面的backup),loadDatabaseSync()拿构造器。2. 三个静态导入改成类型导入 + 运行期加载
以
operational-state-backup.ts为例:import type编译后消失,类型标注照常可用;真正要建数据库的那一行才去加载。session-bundle-policy.ts和operational-target-schema.ts同样处理。3. 四个本地加载函数删掉,改用共享的
operational-state-store.ts和sqlite-session-metadata-store.ts里那两份重复的抑制块整段删除(各减 20 多行),连带删掉只为它服务的createRequire引入。sqlite-runtime-store.ts、sqlite-long-term-memory-store.ts里只惰性不抑制的版本也一并换掉。调用点名字没变(loadDatabaseSync()/loadSqliteModule()),所以函数体以下的代码一行没动。4. 三个零散加载点收编
managed-dependency-environment.ts的内联require('node:sqlite'),以及foreign-session-store.ts/codex-session-adapter.ts的await import('node:sqlite'),都换成共享入口。后两个原本就是惰性的,改动是为了让它们也享受到静音——语义等价,只是把 async 边界去掉了(加载时机不变)。5. 恢复并加固回归测试
packages/storage/src/__tests__/package-import.test.ts在 apache#2710 的批量删测试里被清掉了,之后静态导入才敢一路溜进发布版。这里恢复,并从 1 条断言扩到 3 条:dist/里没有任何模块带静态node:sqlite说明符import type已被抹掉,剩下的必然是值加载第三条是我认为最有价值的:apache#1994 落地当天就会被它拦下来。原来那条只盯 barrel,管不到"哪个文件把它拖进来的"。
没改什么
packages/eval/src/maka-artifacts.ts也有一处静态导入,故意留着。@maka/eval不依赖@maka/storage,接过去要新增跨包依赖;而且它只在maka eval时加载,不在 CLI 启动路径上,跟本 PR 要修的问题无关。该单独一个 PR。退化时间线
package-import.test.tspackage-import.test.ts验证
Node v24.11.1,macOS arm64。
maka --help(已发布0.1.0-beta.1)maka --help(本分支)await import('@maka/storage')改前 / 改后npm test --workspace @maka/storagenpm run typecheck --workspace @maka/storagenpm run lintnpm run build有一项没跑成,如实说明:本想再跑一次真实的
maka run验证运行期也安静,但 dev build 起的 Runtime Host 和机器上已安装的 beta 版起的那个撞了,卡住没出结果,中止了。不过这个场景已被上表第二条测试(构造 store 后 stderr 为空)覆盖,结论不受影响。生成式工具声明
Claude Code(Claude Opus 5)做出了实质贡献:定位退化、编写改动与测试、起草本说明。提交带
Generated-by: Claude Codetrailer。human contributor of record 的 review 尚未完成——本 PR 正是为此而开。