Skip to content

fix(service-storage)!: 引擎写入/读取失败不再伪装成功 —— sys_file 业务真相丢失时响亮失败 (#5216) - #5232

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-5216-storage-metadata-loud-failure
Aug 4, 2026
Merged

fix(service-storage)!: 引擎写入/读取失败不再伪装成功 —— sys_file 业务真相丢失时响亮失败 (#5216)#5232
os-zhuang merged 2 commits into
mainfrom
claude/issue-5216-storage-metadata-loud-failure

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#5216

按 PM 在认领评论里的裁定走 方案 A + 读路径细化:引擎在场时写失败直接抛,读路径区分 miss 与 outage,Map 退化为 engine 缺席时的替身。

一处事实订正:是 6 写 / 2 读,不是 5 写 / 3 读

Issue 正文的 8 行清单本身是准的,PM 派发词里的「5 处写 / 3 处读」是笔误。metadata-store.ts 的 8 处引擎调用是:

方法调用分类
createFileinsert('sys_file')
getFilefindOne('sys_file')
updateFileupdate('sys_file')
deleteFiledelete('sys_file')
createSessioninsert('sys_upload_session')
getSessionfindOne('sys_upload_session')
updateSessionupdate('sys_upload_session')
deleteSessiondelete('sys_upload_session')

6 写 2 读。裁定的形状逐处适用,不受这个计数影响。

写路径(6 处):抛,并且不再往 Map 里写

if (this.engine) 已经把「没接引擎」分流掉了,所以这些 catch 捕获的只可能是已接好的引擎的运行期失败。现在它们统一包成 StorageMetadataStoreError 抛出。

Map 残影的处理:引擎在场时根本不写 Map,而不是「写了再回滚」。理由是这才是问题的机制本身 —— 旧代码是先 this.files.set(...) 再调引擎,所以引擎写丢了以后,紧随其后的 getFile() 从 Map 里读到那条「以为写成功了」的记录,同进程内的自检也看不出异常。回滚只能消除失败那一次的残影,消除不了「同一份数据有两个源」这件事;把 Map 写入整体收进 if (!this.engine) 分支之后,引擎在场时 Map 恒为空,残影在结构上不可能出现,而且读路径也不必再区分「Map 里的是权威还是影子」。这同时让类注释所声称的事第一次成为真的:Map 服务的对象就是 engine === null 的那条分支。

读路径(2 处):miss 与 outage 分开,两处都判定为抛

  • miss(findOne 返回空)—— 设计内的答案,返回 null,REST 层照旧 404。行为不变。
  • outage(findOne 抛)—— 传播出去。

两处都选「抛」而不是「error 日志后回退」,理由按调用方语义:getFile 的三个调用方(/upload/complete/files/:fileId/url/files/:fileId)和 getSession 的三个(chunk、complete、progress)在拿到 null 时一律回 404 FILE_NOT_FOUND / UPLOAD_SESSION_NOT_FOUND。也就是说,静默回退在这里不是「降级到旧数据」,而是把一次引擎故障翻译成「这个文件不存在」——把持久的业务真相报告为缺失,比 500 更糟,且调用方无从分辨。加上写路径改动之后引擎在场时 Map 恒为空,「回退到 Map」实际等价于「返回 null」,也就是等价于那个假 404。多 worker 下更明显:Map 只有本进程的影子,回退会让同一次读在不同 worker 上给出不同答案。

上层调用方:一个都没改,并且这是被验证过的,不是假设

storage-routes.ts 的每个 handler 本来就是 try { … } catch (err) { sendError(res, 500, 'INTERNAL', err?.message) },所以 store 抛出的错误自然落成 500 —— REST 层不需要任何适配。storage-routes.metadata-outage.test.ts 直接驱动 handler 断言了这一点(500 且 success: false,而不是原来的 200)。

仓库内 StorageMetadataStore 的构造点只有 storage-service-plugin.ts:360 一处;packages/cliplugin-devqa/dogfood 只用 StorageServicePlugin,不碰这个 store。所以本 PR 没有修改任何调用方

没有引入新的错误码:一个专门的 STORAGE_METADATA_UNAVAILABLE(503 更诚实)需要在 packages/specERROR_CODE_LEDGER 注册,而本单 ⛔ packages/spec。500 INTERNAL 已经满足「不再是 200」这个验收点,错误码收窄可以另立单。

错误对象

StorageMetadataStoreError(已从包根导出,连同 StorageMetadataOperation 类型):

  • objectName —— sys_file / sys_upload_session
  • operation —— insert / update / delete / findOne
  • cause —— 引擎自己的错误(本包编译在 lib: ES2020,早于 Error.cause,所以是自己声明的字段)
  • message —— 按 AGENTS.md「Degradation log levels」的要求,同时带后果修复。日志级别那条规则本身在这里通过「rethrow」满足,所以没有给 store 加 logger 构造参数;后果与修复写进 message,反而能一路走到 500 的 body 和宿主的日志里。

例:

StorageMetadataStore: sys_file insert failed against the data engine — the sys_file
row was NOT written, so the uploaded bytes have no durable record and are
unaddressable after this process exits. Restore the data engine (connectivity /
permissions / `sys_file` schema migration); the process-local Map fallback serves
only deployments with NO engine wired (tests, dev), so it cannot stand in here.
Cause: Error: …

关于 DURABILITY_CRITICAL_CALLEES:故意不加

AGENTS.md 说发现新的 durability seam 要在同一个 PR 里登记进 scripts/check-durability-degradation-log-level.mjs。这里判断是不该加,理由两条:

  1. 修复方式是把 catch 整个删掉,这个文件里已经没有 catch 可供该 gate 检查;
  2. 该 gate 按被调方法名匹配。这里的被调方法是 insert / update / delete —— 引擎的通用数据面动词。把它们加进词表会命中全仓库每一个包着引擎写入的 catch,而其中绝大多数(包括 storage-routes.ts 自己那些 catch → sendError(500),gate 看不出「回 500 给调用方」也是一种传播)会变成需要 baseline 豁免的假阳性 —— 那正好是把 baseline 变成没人信的清单的做法。

本处的回归保护由新增的单元测试承担。

测试

changeset:.changeset/storage-metadata-loud-failure.md,major,写明了 breaking 的影响面(能观察到的变化是「原本无人察觉的数据丢失现在变成一个 500」,没有需要迁移的东西)。

https://claude.ai/code/session_017MCKJaEomEqg4tvz4SzdNd


Generated by Claude Code

…s success (#5216)
`StorageMetadataStore` wrapped all eight of its `IDataEngine` calls in
`try { … } catch { /* ignore */ }` — no logger, no rethrow, no degradation
flag. `if (this.engine)` had already separated "no engine wired" out, so
those catches could only fire on a RUNTIME failure of a wired engine, and
every one was swallowed behind a process-local Map write that made the
loss invisible inside the same process. A failed `sys_file` insert lost
mostly-permanent business truth (#5202) while the API answered 200.
With an engine wired, the engine is now the only store:
- writes (createFile/updateFile/deleteFile, createSession/updateSession/
deleteSession) propagate as `StorageMetadataStoreError` and mirror
NOTHING into the Map, so no shadow can make a lost write look landed;
- reads (getFile/getSession) separate MISS from OUTAGE — `findOne`
returning nothing still yields `null` (404 unchanged), a thrown engine
error propagates rather than serving this worker's stale local guess;
- the Map is now exactly what the class doc claimed: the engine-absent
stand-in. `new StorageMetadataStore(null)` is unchanged in every respect.
The error message carries the CONSEQUENCE and the FIX per AGENTS.md
"Degradation log levels", and `objectName`/`operation`/`cause` identify
the failure. No route needed editing: the storage handlers already wrap
everything in `catch → sendError(500, 'INTERNAL', …)`, so a lost write is
now a 500 and a read outage is a 500 instead of a false 404.
Tests: metadata-store.test.ts (engine-null behaviour unchanged, no Map
mirroring with an engine present, every write/read outage loud, miss still
null) and storage-routes.metadata-outage.test.ts (the HTTP-visible half).
Both fake engines route `delete` through `assertEngineDeleteDispatch`
(#4550/#5197), which is why `@objectstack/objectql` joins devDependencies.
Fixes#5216
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017MCKJaEomEqg4tvz4SzdNd
@vercel

vercelBot commented Aug 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 4, 2026 11:14am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-storage.

4 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/api/plugin-endpoints.mdx(via @objectstack/service-storage)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/service-storage)
  • content/docs/plugins/packages.mdx(via @objectstack/service-storage)
  • content/docs/releases/implementation-status.mdx(via @objectstack/service-storage)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file tests tooling labels Aug 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependenciesPull requests that update a dependency filedocumentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude