Skip to content

Commit a287d1c

Browse files
baozhoutaoclaude
andauthored
fix(cli): 把本地存储根写成设置服务读的那个 env 名 (#4968) (#5601)
CLI 与设置服务对同一个值用了两个拼写。CLI 自造了 `OS_STORAGE_ROOT`; 设置服务从它自己拥有的命名空间派生 env 名 —— `envKeyOf('storage','local_root')` = `OS_STORAGE_LOCAL_ROOT` —— 而全仓 没有任何地方设置过它。于是两条通道从未相遇:`os serve` 按运维给的根构造 了本地 adapter,`StorageServicePlugin` 在 `kernel:ready` 从 settings 重新 解析,只看到 manifest 的 schema 默认值,就把 adapter 换成了 `./.objectstack/data/uploads`。 所以 `OS_STORAGE_ROOT` 只对一个值生效 —— 恰好等于该默认值的那个,这正是 普通 `pnpm dev` 从没暴露它的原因。其余任何值都是构造完就被丢弃:生产 `/srv/uploads` 被忽略、运维按 backup-restore.mdx 备份到空目录;`dev --fresh` 承诺 tempdir 独占本次运行的全部状态,上传实际落在项目 cwd 且退出后不清理; 每次干净启动都响一条数据丢失级 swap 警告 —— 那条警告是**准确的**,swap 真的发生了,本 commit 不动它,它随 swap 消失而不再响。 修在生产者侧,不在消费者侧加容忍读:`dev.ts` 发布 `OS_STORAGE_LOCAL_ROOT`, `serve.ts` 经单一通道 `resolveStorageLocalRootEnv` 解析根,并与 `os migrate` 的 storage 引导共用,使 CLI 落字节的位置与 server 完全一致。 `OS_STORAGE_ROOT` 经 `readEnvWithDeprecation('OS_STORAGE_LOCAL_ROOT', 'OS_STORAGE_ROOT')` 保留一个 release,每进程 warn 一次,随后移除。旧名供值 时同时回写到新名 —— 设置服务只查 `OS_STORAGE_LOCAL_ROOT`,没有这一步,旧名 部署会原样保留本单要修的缺陷。 不动 `packages/services/service-storage`:swap 谓词是对的(#4096 已修正), 消费缝归 #5536。 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh Co-authored-by: Claude <noreply@anthropic.com>
1 parent cc5b048 commit a287d1c

7 files changed

Lines changed: 253 additions & 15 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
'@objectstack/cli': patch
3+
---
4+
5+
fix(cli): `OS_STORAGE_ROOT` now actually takes effect — renamed to `OS_STORAGE_LOCAL_ROOT`, the name the settings service reads (#4968)
6+
7+
The CLI and the settings service spelled the local storage root differently.
8+
The CLI wrote its own invented name, `OS_STORAGE_ROOT`; the settings service
9+
derives the env name for the same value from the namespace it owns —
10+
`envKeyOf('storage', 'local_root')` = `OS_STORAGE_LOCAL_ROOT` — and nothing in
11+
the repo ever set that. So the two channels never met: `os serve` constructed a
12+
local adapter at the root the operator named, `StorageServicePlugin` re-resolved
13+
from settings at `kernel:ready`, found only the manifest's **schema default**,
14+
and swapped the adapter to `./.objectstack/data/uploads`.
15+
16+
`OS_STORAGE_ROOT` therefore took effect for exactly one value — the one that
17+
happens to equal that default — which is why plain `pnpm dev` never showed it.
18+
Every other value was constructed and then discarded:
19+
20+
- **Production**: `OS_STORAGE_ROOT=/srv/uploads` was ignored and uploads landed
21+
under the process cwd. An operator following `backup-restore.mdx` backed up an
22+
empty directory.
23+
- **`dev --fresh`**: the tempdir was documented to own all state for the run;
24+
uploads actually went to the project cwd and survived process exit.
25+
- Every clean boot logged a data-loss-grade "adapter swapped … existing files
26+
were NOT migrated" warning. That warning was **accurate** — the swap really
27+
happened — and is untouched here. It stops firing because the swap stops.
28+
29+
The fix is at the producer, not as a tolerant read in the consumer: `dev.ts`
30+
publishes `OS_STORAGE_LOCAL_ROOT`, and `serve.ts` resolves the root through one
31+
channel (`resolveStorageLocalRootEnv`), shared with `os migrate`'s storage
32+
bootstrap so the CLI materialises bytes exactly where the server would.
33+
34+
`OS_STORAGE_ROOT` keeps working for **one release** via
35+
`readEnvWithDeprecation('OS_STORAGE_LOCAL_ROOT', 'OS_STORAGE_ROOT')`, warning
36+
once per process, and is then removed. When the legacy name supplies the value
37+
it is also stamped onto the canonical name, because the settings service only
38+
ever looks up `OS_STORAGE_LOCAL_ROOT` — without the stamp a deployment on the
39+
old spelling would keep the original defect in full.
40+
41+
Storage settings now resolve `source: 'env'` at the value the adapter was built
42+
with, so Setup → Settings → File Storage shows the directory actually in use.
43+
No change to `packages/services/service-storage` — the swap predicate is correct
44+
and stays as is.

content/docs/deployment/backup-restore.mdx

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,27 @@ Cloud-managed environments — the open framework does not ship that driver.)
7676

7777
## Backing up uploaded files
7878

79-
The local storage adapter keeps uploads under `OS_STORAGE_ROOT` (default
79+
The local storage adapter keeps uploads under `OS_STORAGE_LOCAL_ROOT` (default
8080
`./.objectstack/data/uploads`). On single-host deployments, include that
8181
directory in the same schedule as the database so records and their
8282
attachments restore to the same point in time. Deployments using an external
8383
storage service (S3-compatible, etc.) inherit that service's durability and
8484
versioning instead.
8585

86+
<Callout type="warn">
87+
**Verify the directory you are backing up actually holds the files.** This
88+
variable was previously named `OS_STORAGE_ROOT`, and on releases before the
89+
rename the CLI and the settings service spelled it differently — so any value
90+
other than the default was discarded at startup and uploads landed in
91+
`./.objectstack/data/uploads` instead of where the variable pointed. A backup
92+
job aimed at the configured path copied an **empty directory**. The old name
93+
still works for one release (with a startup warning), but it now resolves to
94+
the same place the adapter writes. Confirm with **Setup → Settings → File
95+
Storage → Root directory**: it shows the path in effect, and shows it as
96+
locked-by-env when the variable is set. If they disagree, trust the Setup page
97+
and back that path up too.
98+
</Callout>
99+
86100
## The restore drill
87101

88102
Rehearse this on a scratch host **before** go-live, and again after any
@@ -106,8 +120,8 @@ pg_restore --clean --if-exists -d "$OS_DATABASE_URL" backup-2026-07-14.dump
106120
### Provide the original secrets
107121

108122
Set `OS_SECRET_KEY` and `OS_AUTH_SECRET` to the **escrowed originals** — not
109-
freshly generated values. Restore `OS_STORAGE_ROOT` contents if you use local
110-
file storage.
123+
freshly generated values. Restore `OS_STORAGE_LOCAL_ROOT` contents if you use
124+
local file storage.
111125

112126
### Boot from the artifact and verify
113127

content/docs/deployment/environment-variables.mdx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ read at startup unless noted otherwise. Boolean variables accept `true` / `false
5151
| `OS_DATABASE_DRIVER` | enum | inferred | Force a specific driver when the URL is ambiguous. `memory` \| `sqlite` \| `sqlite-wasm` \| `postgres` \| `mongodb`. |
5252
| `OS_DATABASE_SQLITE_JOURNAL_MODE` | enum | `wal` | Journal mode for **file-backed** SQLite. `wal` (default) lets a dev server and CLI commands share one file without blocking each other, and is what makes the `os migrate` occupancy check reliable. Set to `delete` for SQLite's rollback journal — required when the database lives on a **network filesystem** (NFS/SMB), where WAL cannot work. The setting is applied, not merely skipped: `delete` converts a database that already adopted WAL back. Ignored for `:memory:`, for the WASM SQLite driver, and for non-SQLite drivers. A per-datasource `sqliteJournalMode` in driver config outranks it. See [Journal mode](/docs/data-modeling/drivers#journal-mode-wal-and-cross-process-access). |
5353
| `OS_ALLOW_DRIVER_CONNECT_FAILURE` | boolean | `false` | Escape hatch for the driver-connect boot guard. By default a data driver that fails to connect at startup **refuses the boot** — a server that cannot reach its database must not report itself started and then fail every request. The same guard covers a **declared datasource** that objects bind to via `datasource: '…'`, or an `external` one with `validation.onMismatch: 'fail'`: those objects have no fallback datasource, so an unconnected one means they are all dead. Set to `1` to boot anyway, in an explicitly degraded state logged loudly at startup. There is **no reconnection**: whatever failed stays dead for the process lifetime and every query and schema sync routed to it fails. |
54-
| `OS_STORAGE_ROOT` | path | `./.objectstack/data/uploads` | Root directory for the local file storage adapter, relative to the process cwd (used by `os serve`'s default `storage` capability wiring). |
54+
| `OS_STORAGE_LOCAL_ROOT` | path | `./.objectstack/data/uploads` | Root directory for the local file storage adapter, relative to the process cwd (used by `os serve`'s default `storage` capability wiring). This is the same value as **Setup → Settings → File Storage → Root directory**; setting it here pins that field (it shows as locked-by-env). Renamed from `OS_STORAGE_ROOT` — see below. |
55+
| `OS_STORAGE_ROOT` | path || **Deprecated alias for `OS_STORAGE_LOCAL_ROOT`.** Still read for one release, with a startup warning; it will be removed in a future major. Rename it now. Before the rename the two halves of the platform spelled this value differently — the CLI wrote `OS_STORAGE_ROOT` while the settings service read `OS_STORAGE_LOCAL_ROOT` — so **any value other than the default was silently discarded** at startup and uploads landed in `./.objectstack/data/uploads` regardless. If you set `OS_STORAGE_ROOT` on an older release, check where your uploads actually are before assuming a backup covered them. |
5556
| `OS_ARTIFACT_PATH` | path || Path or `http(s)://` URL to a compiled `objectstack.json` artifact to boot the kernel from. |
5657

5758
---

packages/cli/src/commands/dev.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,16 @@ export default class Dev extends Command {
182182
// ── --fresh: ephemeral OS_HOME under the OS tempdir ─────────────
183183
// Creates a unique scratch dir that owns ALL persistent state for
184184
// this run: the SQLite DB (via OS_HOME → <home>/data/...), the
185-
// storage-service uploads root (OS_STORAGE_ROOT), and any other
185+
// storage-service uploads root (OS_STORAGE_LOCAL_ROOT), and any other
186186
// state plugins keyed off OS_HOME. Auto-deleted on exit.
187+
//
188+
// The uploads root MUST be published under the name the settings
189+
// service derives for it — `envKeyOf('storage','local_root')` (#4968).
190+
// Under the CLI's old private spelling (`OS_STORAGE_ROOT`) the settings
191+
// side saw no env value, fell back to the manifest default, and swapped
192+
// the adapter to `./.objectstack/data/uploads` at kernel:ready — so
193+
// `--fresh` uploads landed in the PROJECT CWD and outlived the run,
194+
// which is the opposite of what this block promises.
187195
let freshHome: string | undefined;
188196
let freshDbUrl: string | undefined;
189197
let freshStorageRoot: string | undefined;
@@ -245,7 +253,7 @@ export default class Dev extends Command {
245253
...(seedAdmin && flags['admin-email'] ? { OS_SEED_ADMIN_EMAIL: flags['admin-email'] } : {}),
246254
...(seedAdmin && flags['admin-password'] ? { OS_SEED_ADMIN_PASSWORD: flags['admin-password'] } : {}),
247255
...(freshHome ? { OS_HOME: freshHome } : {}),
248-
...(freshStorageRoot ? { OS_STORAGE_ROOT: freshStorageRoot } : {}),
256+
...(freshStorageRoot ? { OS_STORAGE_LOCAL_ROOT: freshStorageRoot } : {}),
249257
...(effectiveDb ? { OS_DATABASE_URL: effectiveDb } : {}),
250258
...(flags['database-driver'] ? { OS_DATABASE_DRIVER: flags['database-driver'] } : {}),
251259
...(flags['database-auth-token'] ? { OS_DATABASE_AUTH_TOKEN: flags['database-auth-token'] } : {}),

packages/cli/src/commands/serve-storage-capability.test.ts

Lines changed: 111 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,9 @@
2525
* write it are now told, by `lintUnknownAuthoringKeys`.
2626
*/
2727

28-
import { describe, it, expect } from 'vitest';
29-
import { resolveStorageCapabilityArg } from './serve.js';
28+
import { afterEach, describe, it, expect, vi } from 'vitest';
29+
import { _resetEnvDeprecationWarnings } from '@objectstack/types';
30+
import { resolveStorageCapabilityArg, resolveStorageLocalRootEnv } from './serve.js';
3031

3132
describe('resolveStorageCapabilityArg', () => {
3233
it('builds options StorageServicePlugin actually reads', () => {
@@ -45,7 +46,10 @@ describe('resolveStorageCapabilityArg', () => {
4546
expect(options).not.toHaveProperty('root');
4647
});
4748

48-
it('honours OS_STORAGE_ROOT, which the old shape discarded', () => {
49+
// Renamed from "honours OS_STORAGE_ROOT" (#4968): this case never read env,
50+
// it passes the root as an argument. Naming it after a variable it does not
51+
// touch is how the env channel went unexamined while the shape looked pinned.
52+
it('honours an explicit root, which the old shape discarded', () => {
4953
const { options, localRoot } = resolveStorageCapabilityArg('/srv/uploads');
5054
expect(options).toEqual({ adapter: 'local', local: { rootDir: '/srv/uploads' } });
5155
expect(localRoot).toBe('/srv/uploads');
@@ -77,3 +81,107 @@ describe('resolveStorageCapabilityArg', () => {
7781
});
7882

7983
});
84+
85+
/**
86+
* #4968 — the env CHANNEL, which #4096 left split.
87+
*
88+
* #4096 pinned the option shape and the tests above went green, but the value
89+
* still could not reach the settings service: the CLI wrote `OS_STORAGE_ROOT`
90+
* and the settings service reads `envKeyOf('storage','local_root')` =
91+
* `OS_STORAGE_LOCAL_ROOT`, which nothing in the repo ever set. So settings saw
92+
* only the manifest's schema default and swapped the adapter at `kernel:ready`
93+
* — `OS_STORAGE_ROOT` took effect for exactly one value (the one equal to that
94+
* default) and `dev --fresh` wrote uploads into the project cwd.
95+
*
96+
* The stamp assertion is the load-bearing one. Returning the legacy value is
97+
* only half the migration; if the canonical name is not also SET, a deployment
98+
* on the old spelling keeps the original bug in full, silently.
99+
*/
100+
describe('resolveStorageLocalRootEnv (#4968)', () => {
101+
const CANONICAL = 'OS_STORAGE_LOCAL_ROOT';
102+
const LEGACY = 'OS_STORAGE_ROOT';
103+
const originalCanonical = process.env[CANONICAL];
104+
const originalLegacy = process.env[LEGACY];
105+
106+
afterEach(() => {
107+
if (originalCanonical === undefined) delete process.env[CANONICAL];
108+
else process.env[CANONICAL] = originalCanonical;
109+
if (originalLegacy === undefined) delete process.env[LEGACY];
110+
else process.env[LEGACY] = originalLegacy;
111+
_resetEnvDeprecationWarnings();
112+
vi.restoreAllMocks();
113+
});
114+
115+
it('reads the canonical name the settings service derives, quietly', () => {
116+
delete process.env[LEGACY];
117+
process.env[CANONICAL] = '/srv/uploads';
118+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
119+
120+
expect(resolveStorageLocalRootEnv()).toBe('/srv/uploads');
121+
expect(warn).not.toHaveBeenCalled();
122+
// The whole point: this is the name `envKeyOf('storage','local_root')`
123+
// produces, so the settings service resolves source:'env' at this value.
124+
expect(process.env[CANONICAL]).toBe('/srv/uploads');
125+
});
126+
127+
it('still reads the legacy name AND stamps it onto the canonical one', () => {
128+
delete process.env[CANONICAL];
129+
process.env[LEGACY] = '/srv/legacy-uploads';
130+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
131+
132+
expect(resolveStorageLocalRootEnv()).toBe('/srv/legacy-uploads');
133+
// Without this line a legacy deployment keeps the exact defect #4968
134+
// describes: adapter built at /srv/legacy-uploads, settings still on its
135+
// schema default, adapter swapped away at kernel:ready.
136+
expect(process.env[CANONICAL]).toBe('/srv/legacy-uploads');
137+
138+
expect(warn).toHaveBeenCalledTimes(1);
139+
const msg = String(warn.mock.calls[0][0]);
140+
expect(msg).toContain(LEGACY);
141+
expect(msg).toContain(CANONICAL);
142+
expect(msg).toContain('deprecated');
143+
});
144+
145+
it('feeds the capability arg from the legacy name end to end', () => {
146+
delete process.env[CANONICAL];
147+
process.env[LEGACY] = '/srv/legacy-uploads';
148+
vi.spyOn(console, 'warn').mockImplementation(() => {});
149+
150+
const { options, localRoot } = resolveStorageCapabilityArg(resolveStorageLocalRootEnv());
151+
expect(options).toEqual({ adapter: 'local', local: { rootDir: '/srv/legacy-uploads' } });
152+
expect(localRoot).toBe('/srv/legacy-uploads');
153+
// Constructor side and settings side now name the same directory, which is
154+
// what makes `needsStorageSwap` answer false instead of swapping + warning.
155+
expect(process.env[CANONICAL]).toBe(localRoot);
156+
});
157+
158+
it('lets the canonical name win when both are set, without warning', () => {
159+
process.env[CANONICAL] = '/srv/canonical';
160+
process.env[LEGACY] = '/srv/legacy';
161+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
162+
163+
expect(resolveStorageLocalRootEnv()).toBe('/srv/canonical');
164+
expect(process.env[CANONICAL]).toBe('/srv/canonical');
165+
// The stamp must never overwrite an explicitly-set canonical value, and the
166+
// legacy variable is left exactly as the operator wrote it.
167+
expect(process.env[LEGACY]).toBe('/srv/legacy');
168+
expect(warn).not.toHaveBeenCalled();
169+
});
170+
171+
it('sets nothing when neither name is set, so the default still applies', () => {
172+
delete process.env[CANONICAL];
173+
delete process.env[LEGACY];
174+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
175+
176+
expect(resolveStorageLocalRootEnv()).toBeUndefined();
177+
// Must NOT stamp a default: an env-locked value would show up in Setup as
178+
// locked-by-env and take the root out of the admin's hands for no reason.
179+
expect(process.env[CANONICAL]).toBeUndefined();
180+
expect(warn).not.toHaveBeenCalled();
181+
182+
// Unset falls through to the resolver default, which equals the manifest
183+
// default — this is exactly why plain `pnpm dev` never showed the bug.
184+
expect(resolveStorageCapabilityArg(resolveStorageLocalRootEnv()).localRoot)
185+
.toBe('.objectstack/data/uploads');
186+
});
187+
});

0 commit comments

Comments
 (0)