Repository navigation
Commit 5cdaddb
fix(service-datasource)!: a datasource update whose new configuration fails to connect stores none of it (#29981)
<!-- claude-projects-attribution: {"github_login":"os-support-ai"} -->
_Requested by **Yibo Wang** · [project
thread](https://claude.ai/code/project/chan_01BVUyyPie8gHwoPM18iABwB?thread=cmsg_01BVUyyPie8gHwoPM18iABwBLN3AWdL2vTrzytbTbs93mx)_
Fixes #29879
Clause-②: yes (narrowing)
Metadata-protocol: none
Skills-docs: none — no file under `skills/**` or `content/docs/**`
teaches what a datasource update stores when its rebuild fails, or what
a blank `secret` does at the admin doors; the fail-closed paragraph in
`external-datasources.mdx` (§4 Credentials) stays true.
**BREAKING** accept-set narrowing at the datasource admin doors
(`@objectstack/service-datasource`): a whitespace-only `secret` is
refused.
## 维护者速读
- **改了什么**:`PATCH /api/v1/datasources/NAME`
在新配置连不上、旧连接池被保留时,现在什么都不落盘:数据源记录、`external.credentialsRef`、同一次 PATCH
里的其它键都保持原样,为这次 PATCH 新建的 sys_secret 行会被删掉。接口回答不变(200、`status:
"error"`、「新配置未生效」),现在这句话是真的。另外,全空白的 `secret`(`" "`、制表符加换行)和空串一样在入口回
400「Nothing was written」,用的是同一个守卫和同一句话。
-
**为什么改**:以前先存记录、换凭据、删旧凭据,再重建连接池;重建失败时旧池继续服务,接口说「未生效」,但下次重启读到的是失败的新凭据,能用的旧凭据已经被删了(#29879,QA
记录 #29877)。
- **风险与回滚**:中低。成功路径的结果不变(新凭据存盘并生效,旧凭据退役),只是顺序从「先存后连」变成「先连后存」,失败时不存。自己实现
`reregisterPool` 的宿主不回报结果时行为与以前相同。回滚即 revert。
-
**先看**:`packages/services/service-datasource/src/datasource-admin-service.ts`
的 `updateDatasource` 尾部(`previousPoolKept` 分支、`putDatasourceRecord`
之后才删旧凭据)与 `assertSecretNotEmpty`;`datasource-admin-plugin.ts` 的
`reregisterPool` 如何判断旧池被保留。
- **两分钟验证**:`pnpm --filter @objectstack/service-datasource exec vitest
run src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts
src/__tests__/datasource-empty-secret-refusal.test.ts`,预期 `Tests 17
passed (17)`(main 上 7 条红:3 条「重启后仍连得上」、4 条空白 secret;对照两边都绿)。
- **需要你做的**:无,席位落地(这是收窄,按规矩先做合同审查)。
## What changes
- **The update stores nothing of a configuration that never came into
force.** `updateDatasource` used to seal the new secret, remove the
working one, store the merged record, and only then rebuild the pool. It
now seals the new secret (the rebuild dereferences the merged record's
`credentialsRef`), rebuilds, and only then decides:
- the `reregisterPool` seam reports `previousPoolKept: true` → nothing
is stored, the `sys_secret` row sealed for this patch is removed, no
cluster signal is published (there is nothing for a peer to converge
on), and the answer is `toSummary(existing)`: `200`, `status: "error"`,
the reason ending "the new configuration is NOT in force";
- otherwise → the merged record is stored, and the replaced secret is
retired only **after** that write. A `putDatasourceRecord` that throws
now takes the newly sealed secret back out instead of leaving the record
pointing at a removed one.
- **Which of the two routes, and why:** "never write until the reconnect
succeeds", so the record has ONE write path (one `putDatasourceRecord`,
after the pool settles). The restore route would have written the record
twice on failure, and its second write can fail too.
- `active: false` and the no-connectivity-change register keep their
order (store, then pool), unchanged.
- **The seam reports the outcome.**
`DatasourceAdminServiceConfig.reregisterPool` may now answer `{
previousPoolKept: boolean }` (new `ReregisterPoolOutcome` in
`datasource-admin-service.ts`, not exported from the index). A seam that
answers nothing reads as before: the record is stored whatever the
rebuild did. This is the widening half of `Clause-②: yes (narrowing)`.
- **The plugin's seam answers it from the state the rebuild left.**
`DatasourceAdminServicePlugin`'s `reregisterPool` reads: the new
configuration is not `available` AND a driver still answers the name.
That driver is the previous pool `reconnect` put back, the only driver a
failed rebuild leaves registered (a rebuild with no live pool to keep is
a plain `connect`, which registers nothing on failure). It reads the
state, not the verdict the rebuild started from: after a first failed
PATCH the kept pool serves under a FAILED verdict, and a second failing
PATCH must store nothing either (pinned). On a kept pool the plugin's
`livePoolRecords` entry goes back to `previous`, so a later cluster
convergence compares the shared row against the configuration actually
serving. A `reconnect` that throws after restoring the pool is reported
as kept (logged at `warn`, as the admin service logged it before); one
that throws with nothing kept rethrows.
- **A binder that rewrites in place.** The shipped `sys_secret` binder
mints a fresh handle per write, so removing the new row restores the
binding exactly. For a host binder that reseals under the SAME
reference, the working credential is already overwritten: the row is
never removed (it is the binding the stored record names) and the loss
is logged once at `error` with the remedy.
- **Blank secrets.** `assertSecretNotEmpty` now refuses a `secret` whose
value is whitespace only, on create and on update, with the same message
(`The \`secret\` is empty. … Nothing was written.`) and the same `400
DATASOURCE_ADMIN_ERROR`. No second guard. A value with whitespace around
other characters is still sealed as sent.
- `datasource-connection-service.ts` is unchanged: `reconnect` already
kept the previous pool; the defect was the store write before it.
- **Changeset**
`.changeset/29879-datasource-update-kept-pool-stores-nothing.md`:
`patch` on `@objectstack/service-datasource`, both declaration lines,
BREAKING banner, the remedy, ADR-0087 marker `not-required
(no-migration-prescription)` (as #28928).
**Why `Clause-②: yes (narrowing)` and not the claim's `no`.** The
narrowing is the blank-secret refusal, declared as #28928 declared the
empty string (`no (narrowing)` there). This diff also widens an exported
type (`DatasourceAdminServiceConfig.reregisterPool`'s answer), so the
value is `yes`; `yes (narrowing)` is the spelling for a diff that widens
one surface and narrows another (`scripts/pm/clause2-line.mjs`).
Metadata-protocol stays `none`: `secret` is the doors' credential
channel, not an authorable key, so the level is `patch`.
## Tests
- **New**
`src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts`: the
REAL `DatasourceAdminServicePlugin` and
`ExternalDatasourceServicePlugin` behind the REAL admin routes, with the
REAL `sys_secret` binder, over one in-memory store (`sys_metadata` +
`sys_secret`) that outlives a boot. A restart is a second boot over the
same store. The pool connects only to database `qa` with the real
password.
- the card's reproduction: create with the real password → `PATCH
{secret: wrong, label}` → `200`, `status: "error"`, "NOT in force" → the
stored record and the `sys_secret` ids equal what they were before the
PATCH → `/test` ok on the kept pool → restart → `/test` ok, and the
restart built its pool with the real password;
- a failing `config` change: the same, the stored config is the one
serving;
- a second failing PATCH after a first one: still nothing stored;
- control: a secret that connects is in force and stored, the replaced
`sys_secret` row is gone, and the restart connects.
- **Extended** `src/__tests__/datasource-empty-secret-refusal.test.ts`
(the #28928 harness, real routes over the real service, every seam call
recorded): `PATCH` with `" "` and with `{ value: "\t\n" }`, `POST` with
`" "` and `"\t\n"` → `400 DATASOURCE_ADMIN_ERROR`, message contains
`secret` and "Nothing was written.", no seam call at all. New control: a
secret with whitespace around other characters is sealed as sent.
<details><summary>Red on main, green on head, two ablations</summary>
- **Main** (the two source files restored from `18112bfa`, the tests at
head): `Tests 7 failed | 10 passed (17)`. The three kept-pool pins
received the card's state, for example `external.credentialsRef`
`sys_secret:sec_2` for `sec_1`, `label: "Renamed in the same patch"`
stored, `secretIds` `[sec_2]` for `[sec_1]`; the four blank-secret pins
received `{ status: 200 | 201, success: true }`. Every control green.
Restored: `git diff HEAD` empty.
- **Head:** `Tests 17 passed (17)`.
- **Ablation 1** (`scripts/ablation-replace.mjs`, `if (previousPoolKept)
{` → `if (previousPoolKept && 'ABLATED_29879' === '') {`): `Tests 3
failed | 1 passed (4)`, exactly the three kept-pool pins; the
good-secret control green. Restore: blob `cea697a4` equals HEAD's.
- **Ablation 2** (`value.trim() !== ''` → `value !== ''`, the #28928
guard): `Tests 4 failed | 9 passed (13)`, exactly the four blank-secret
pins. Restore: blob `cea697a4` equals HEAD's.
</details>
## Acceptance notes
- **The other caller of the seam.** `convergePool` (a peer's rebuild
after a cluster signal) ignores the answer, as it should: it writes no
record. It benefits from the `livePoolRecords` fix only.
- **The pre-#13804 fallback.** A host without the `reregisterPool` seam
keeps the incumbent pool through the idempotent register and stores the
new record, as before; nothing in that path says "NOT in force".
- **A rebuild with no live pool to keep** (the datasource was inactive,
or its last connect failed) still stores the new record when it fails:
there is no serving configuration to protect, and the answer does not
claim one.
- **SDK/script callers** that send a whitespace-only `secret` now get
the 400; the remedy is in the message and the changeset.
## Verification
All readings at head `2e46f163` (four commits on `origin/main`
`18112bfa`; main had not moved when the gates and the dogfood ran).
<details><summary>Package suite, typecheck, lint</summary>
- `pnpm --filter @objectstack/service-datasource test`: `Test Files 71
passed (71)`, `Tests 1169 passed (1169)`, exit 0 (taken before the
ledger commit, which touches only
`scripts/engine-double-contract.pinned.json`).
- `pnpm --filter @objectstack/service-datasource typecheck` (tsc over
`src`, tests included): exit 0.
- The repo's `pnpm lint` command scoped to
`packages/services/service-datasource`: exit 0, no findings.
</details>
<details><summary>Derived gates (`dispatch-gates.mjs --commands`, 69
families)</summary>
- 68 exit 0, `check-adr-0087-registration --base origin/main` and
`check-changeset-no-major --base origin/main` included.
`check:engine-double-contract` first answered 1 for the new restart
rig's doubles (`RETAINED [delete] [findOne] [update]`), then 0 after
`--write` and the ledger commit; the 8 families that commit added all
exit 0. `check-plugin-teardown-shape --self-test` exit 0 after fetching
its pinned fixture commit.
- `check:dual-build-cjs-loads`: exit 3, PREREQUISITE NOT MET (it needs
dists of packages outside this closure). NOT MEASURED locally; CI's.
- `dispatch-gates --ran`: `69 derived, 68 run, 1 NOT-MEASURED, 0 UNRUN`.
- Repo-wide `pnpm lint` and the CI-only families are CI's; see the PR's
check-runs.
</details>
Dogfood battery (owed: `!` title and the `(narrowing)` arm), once at
`2e46f163` after `pnpm turbo run build
--filter='@objectstack/dogfood...'` (69 tasks, exit 0): `pnpm --filter
@objectstack/dogfood test`, `Test Files 415 passed | 3 skipped (418)`,
`Tests 3432 passed | 28 skipped (3460)`, lock VERDICT command-exit 0.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01WGufzfDo764sexAMz4Feui)_
<!-- signed: domain:server#6 · os-support-ai ·
session_01WGufzfDo764sexAMz4Feui -->
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c2be734 commit 5cdaddb
6 files changed
Lines changed: 503 additions & 29 deletions
File tree
- .changeset
- packages/services/service-datasource/src
- __tests__
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
33 | 36 | | |
34 | 37 | | |
35 | 38 | | |
| |||
135 | 138 | | |
136 | 139 | | |
137 | 140 | | |
| 141 | + | |
138 | 142 | | |
139 | 143 | | |
140 | 144 | | |
141 | 145 | | |
142 | | - | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
143 | 153 | | |
144 | 154 | | |
145 | 155 | | |
| |||
149 | 159 | | |
150 | 160 | | |
151 | 161 | | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
152 | 168 | | |
153 | 169 | | |
154 | 170 | | |
| |||
169 | 185 | | |
170 | 186 | | |
171 | 187 | | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | | - | |
178 | | - | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
179 | 197 | | |
180 | 198 | | |
181 | 199 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
0 commit comments