You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 54c3ce1
Browse filesBrowse the repository at this point in the historyBrowse files
fix(service-storage): chunks sent in parallel to one chunked upload each record their part (#22332) (#22348)
Fixes#22332
Clause-②: no
## What was wrong
The chunk door (`PUT
/storage/upload/chunked/:uploadId/chunk/:chunkIndex`) read the session
row, merged its part into the parts list in memory (`recordChunk`) and
wrote the whole record back with an unconditional by-id update. Two PUTs
to one upload at once both read the same record, and the later write
erased the earlier part: both answered `200`, `sys_upload_session.parts`
kept one part, `uploaded_chunks` / `uploaded_size` counted one chunk,
and the completion door refused the upload `409` naming the lost chunk.
## The atomic route taken, and why
**A compare-and-set on the row's three progress columns, with a bounded
re-read-and-merge retry.** This is the route the store already supports;
it needs no new object, no new field and no `packages/spec` change.
- **On a wired engine** it is the engine's own conditional update:
`engine.update('sys_upload_session', progress, { where: { parts,
uploaded_chunks, uploaded_size, id }, multi: true, context })`. The
update-dispatch module (`resolveEngineUpdateDispatch`,
`@objectstack/metadata-core`) names exactly this shape, an id in `where`
beside further keys with `multi: true`, as "the compare-and-set
spelling". It routes to `driver.updateMany`, which evaluates the guard
in the same statement that writes and answers the matched-row count.
driver-sql issues one `UPDATE … WHERE` with the tenant scope applied;
driver-memory matches and writes in one synchronous step. Every driver's
`updateMany` (sql, memory, turso, mongodb) resolves a count.
- **On the engine-absent stand-in** it is one synchronous
compare-and-set on the Map.
- **Why these columns are the guard:** they are exactly the values the
merge read, so the comparison is exact. `updated_at` as a version has
millisecond grain, so two writes in one millisecond would still match. A
per-part row would be a new object, a schema change, and was not needed.
- ⛔ No process-local lock: a second server process would defeat it. ⛔
The completion door's guard is untouched.
The store side is `updateSessionProgressIfUnchanged` in
`metadata-store.ts`. It is a module export that is not re-exported from
the package entry, following the `organizationOutOfWriteReach` pattern,
so `StorageMetadataStore`'s public face does not grow.
The door merges and writes up to `CHUNK_RECORD_ATTEMPTS` (16) times. A
conditional write loses only to a write that landed between its read and
itself, so each lost attempt is another chunk recorded.
- **On exhaustion** the door answers `409 RESOURCE_CONFLICT` with
`error.details: { chunkIndex, attempts }`. The chunk's bytes are stored
but the record does not hold them, and re-sending the chunk replaces its
slot. It never answers a silent `200`.
- **When the conditional write cannot reach the row** (it misses, and
the row still holds the progress the write was conditioned on), the door
answers `500` at once. It does not loop into a `409` that would tell the
uploader to retry something that cannot succeed.
## Measured: what the store supports (H2)
A probe against the real `ObjectQL` over `SqlDriver` (better-sqlite3,
`:memory:`) at `e36ee5351b`:
| conditional update | answer |
| :--- | ---: |
| guard as read | 1 |
| stale guard | 0 |
| guard on an 8-part `parts` string | 1 |
| guard of NULL columns (lowered to `IS NULL`) | 1 |
| row stamped org_B, acting organization org_A | 0, no throw |
| the same row, acting organization org_B | 1 |
| missing row | 0 |
## Measured: the reproduction, before and after
`pnpm --filter @objectstack/service-storage exec vitest run
--maxWorkers=2 src/chunk-part-record-concurrency.test.ts`
**Before**, the pins on `main` at `e36ee5351b` (which carries the
completion guard): 8 failed and 2 passed; the 2 that passed are the
sequential controls.
- The stand-in and the real engine alike lost parts: `every concurrently
sent chunk is in the record: expected [ 1 ] to deeply equal [ +0, 1 ]`
(2 concurrent), and `expected [ +0 ] to deeply equal [ +0, 1, 2, 3, 4,
5, 6, 7 ]` (8 concurrent).
- The parallel upload's completion was refused `409` with
`missingChunks: [1]`.
- With a competing write landing between the read and the write, the
door answered `{"success":true,…}` (`expected 200 to be 409`), and the
record held `[ +0 ]`: the competitor's part was erased.
**After**, at `006344eb50`: 24 passed (24). Both stores pass the 2 and 8
concurrent pins (every part, every eTag and size, `uploaded_chunks`,
`uploaded_size`, and the progress door), the parallel completion `200`
with the whole file, the sequential control (the same answers and
record, one session write per chunk), and the exhaustion pin (`409
RESOURCE_CONFLICT` after exactly 16 reads, with every competitor's part
kept). The store-level pins also pass on both stores: the write lands on
an unchanged row, misses once any one progress column moved, and misses
on a gone row. On the real engine, the call shape is pinned (dispatch
verdict `multi`, the same `{ tenantId, isSystem }` context), another
organization's row is not reached, a non-count answer is refused loudly,
and an unreachable row answers `500` once.
**Over HTTP**, a real boot (`bootStack`, sqlite-wasm):
`packages/qa/dogfood/test/storage-chunked-parallel-parts.dogfood.test.ts`
passes 2 of 2 (2 concurrent PUTs, then progress and completion `200`; 8
concurrent PUTs, then progress). With the sibling
`storage-chunked-resume-integrity.dogfood.test.ts`, 6 passed (6) at
`006344eb50`.
## Ablations
Each mutation went through `scripts/ablation-replace.mjs`: the anchor
hit 1 to 0, the blob changed, and the restore was proven with blob ==
HEAD and an empty `git diff HEAD`. The runner also carried its own
`EXIT`/`INT`/`TERM` restore over absolute paths. The control run with no
mutation passed 24 of 24.
| mutation | pins turned red |
| :--- | :--- |
| M1: the stand-in's comparison removed | 7, all stand-in: 2 and 8
concurrent, the parallel completion, exhaustion, and 3 column-moved pins
|
| M2: the engine guard replaced by an unchanging column | 8, all engine:
2 and 8 concurrent, the parallel completion, exhaustion, 3 column-moved
pins, and the where-shape pin |
| M3: the door treats a lost write as landed | 9: on both stores, 2 and
8 concurrent, the parallel completion and exhaustion; plus the
unreachable-row `500` |
| M4: exhaustion falls through to `200` | 2: exhaustion, on both stores
|
| M5: a non-count engine answer accepted | 1 |
| M6: the unreachable-row check removed | 1 |
| M7: the stand-in lands on a missing row | 1 |
| M8: the organization scope dropped from the conditional write | 2 |
The dogfood pin reads `@objectstack/service-storage` from `dist/`, so
its ablation included a rebuild.
- **Mutation leg:** M3, then a rebuild. `ablation-dist-preflight` found
the marker in `dist/index.js` and `dist/index.cjs`, and 2 of 2 failed
(the progress fell short of `uploadedChunks`).
- **Restore leg:** a rebuild, after which the marker was absent from all
6 built files and the tree was clean against HEAD. 2 of 2 passed.
## Tests and gates (HEAD `006344eb50`)
- `@objectstack/service-storage`: 46 files and 773 tests passed.
`typecheck` passed (`tsc`, the scripts project, and
`check:test-typecheck: OK`). `--listFiles` shows the new test file is in
the test-layer program.
- `@objectstack/dogfood`: `typecheck` passed.
- Two fake engines now answer a declared predicate update with its
matched-row count, as the real engine does:
`tenant-audit-update-delete-half-repairs.test.ts` and
`storage-routes.metadata-outage.test.ts`. The chunk-door pin in the
first now expects the conditional `where` with `multi: true`, under the
same context.
- `check:tenant-audit-census`: the new conditional write is one more
engine write call site (236 to 237). The census was regenerated (`node
scripts/tenant-audit-census.mjs --write`), and the page's hand-written
prose figures were moved with it. The gate and its self-test pass.
- Lint, narrowed and stated as a measurement. ① Population: the 6
touched TypeScript files, all inside the `eslint.config.mjs` glob
`**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}`. ② `--format json` linted 6 files
with 0 errors and 0 warnings. ③ Invariance: the config sets no
`parserOptions.project` (`--print-config` gives
`{"ecmaVersion":"latest","sourceType":"module"}`), so linting is not
type-aware and this diff cannot move a verdict on an untouched file. The
repo-wide `pnpm lint` is CI's.
- The derived gate families (`dispatch-gates --commands`, derived at
`006344eb50`) were run after the last commit, with exit codes captured
before any pipe. `dispatch-gates --ran` reads `97 derived, 97 run, 0
NOT-MEASURED, 0 UNRUN` (a derived zero: every family recorded `exit 0`).
`pnpm check:error-status-conformance` was run by hand and exited 0: `✓
every derivable runtime status is documented, and every documented
status is reachable.` A few verdict lines:
- `check-engine-double-contract: OK — 983 pinned, 129 in the DEBT
ledger, 3 exempt.`
- `check-nul-bytes: OK (scanned 10310 text file(s) …; no raw ASCII
control bytes).`
- `✓ check-tenant-audit-census: OK -- 237 write call sites certified …`
- `check-test-source-alias OK — 73 packages with tests scanned …`
- `✓ check:dual-build-cjs-loads — 106 published require entry point(s)
across 66 package(s) load …`. Its first run answered `PREREQUISITE NOT
MET` (no `dist/` for 8 packages), which is not a measurement. It passed
after a full cache-backed `turbo run build`.
- ✓ This diff introduces no `major` bump. (`check-changeset-no-major`)
## Acceptance notes
- **Clause-② stays `no`, as the claim declared.** No accept set, export
or public signature moves. The one new answer, `409` after 16 lost
record writes, replaces a `200` that had recorded nothing.
- **The progress write is now a predicate update.** On an engine-backed
store, the chunk door's write publishes the bulk `data.records.updated`
event (a count) where it published `data.record.updated`, and its
after-hooks go through the bulk per-row path. Nothing in the tree
registers an update hook on `sys_upload_session` or subscribes to its
record events, and the config-change audit excludes the object.
- **Observation, read-only and not measured, not filed** (承接者:无). The
chunk door's upload-limit check judges the record as it was read.
Concurrent PUTs can therefore each pass it against the same stale total,
and the backend can briefly hold more bytes than `maxUploadBytes`. The
completion door still refuses any upload whose held bytes differ from
its declared size, and the init door bounds that size.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
fix(service-storage): chunks sent in parallel to one chunked upload each record their part
6
+
7
+
Clause-②: no
8
+
9
+
`PUT /storage/upload/chunked/:uploadId/chunk/:chunkIndex` merges its chunk into the upload session's record of the chunks it holds (`parts`, `uploaded_chunks`, `uploaded_size` on `sys_upload_session`). It read that record, merged in memory and wrote the whole record back, so two chunk PUTs to one upload at the same time both answered `200` while the record kept only one of them: `GET …/progress` undercounted, and the completion was refused `409 RESOURCE_CONFLICT` naming the chunk the record had lost until the client sent it again.
10
+
11
+
The record is now written with a compare-and-set: the write lands only while the row still holds the progress the chunk door read, and when another chunk's write landed first the door reads the record again and merges again. On a wired data engine this is the engine's own conditional update, evaluated in the same statement that writes, so it holds across server processes. Every chunk sent in parallel is recorded, and a parallel upload completes on its first completion. A sequential upload is unchanged.
12
+
13
+
A chunk whose record write loses to another write on 16 attempts in a row is refused `409 RESOURCE_CONFLICT`, with `error.details``{ chunkIndex, attempts }`: its bytes are stored, but the upload does not hold it. Send that chunk again.
Copy file name to clipboardExpand all lines: content/docs/permissions/tenant-audit-census.mdx
+23-23Lines changed: 23 additions & 23 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -83,7 +83,7 @@ what moved this page's population from 225 to 227; nothing about the two sites
83
83
changed, only whether this instrument could see them.
84
84
85
85
**The expensive failure direction is a keyword.** Sites whose receiver the author
86
-
typed `any` have no type to read, and there are 50 of them — just over a fifth
86
+
typed `any` have no type to read, and there are 51 of them — just over a fifth
87
87
of the population, concentrated in exactly the seed and bootstrap paths this
88
88
control exists for. Scoring an unreadable receiver as "not an engine" would have
89
89
dropped every one of them silently, with a clean exit and a smaller number that
@@ -122,7 +122,7 @@ are reported as `undecidable` rather than assumed either way.
122
122
123
123
The same holds twice over for the context. An options argument spelled as a
124
124
literal can be read; one spelled `options`, `{ ...opts }`, or handed through a
125
-
forwarding shim cannot, and **52 of the 236 sites are spelled that way**. A
125
+
forwarding shim cannot, and **53 of the 237 sites are spelled that way**. A
126
126
context resolved from an inline literal or a local `const` can be tested for
127
127
`isSystem`; one arriving from a helper call cannot.
128
128
@@ -150,8 +150,8 @@ now **0**: nothing on this surface threads a context that provably lacks the fla
150
150
151
151
**"No tenant context" counted sites it had not read.** An options argument the
152
152
walker could not parse was folded into the same bucket as one it had read and
153
-
found empty. That published **60 sites "carrying no tenant context at all"**
154
-
when 8 said so and 52 were simply unread — an over-claim in the *alarming*
153
+
found empty. That published **61 sites "carrying no tenant context at all"**
154
+
when 8 said so and 53 were simply unread — an over-claim in the *alarming*
155
155
direction, on the very figure this page tells other cards to cite. `carries` is
156
156
now three-valued, and an unreadable argument can never contribute to the
157
157
provable count.
@@ -187,10 +187,10 @@ reproduce them. Where it disagrees, it disagrees on the page:
187
187
188
188
| carried figure | where it survives | this census |
189
189
| :--- | :--- | ---: |
190
-
| 175 write call sites | quoted in the merged changeset |**236**|
191
-
| 24 carrying no tenant context | quoted in the merged changeset |**2** provable and tenancy-enabled; **31** more whose options argument is unreadable |
192
-
| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card |**156 of 236** decidable, **80** undecidable |
193
-
| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration**|**not reproduced**: 125 decidably elevated, 0 decidably not, 103 undecidable |
190
+
| 175 write call sites | quoted in the merged changeset |**237**|
191
+
| 24 carrying no tenant context | quoted in the merged changeset |**2** provable and tenancy-enabled; **32** more whose options argument is unreadable |
192
+
| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card |**157 of 237** decidable, **80** undecidable |
193
+
| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration**|**not reproduced**: 125 decidably elevated, 0 decidably not, 104 undecidable |
194
194
| 141 and 132, two independent re-derivations | the card that filed this work | — |
195
195
196
196
**The differences are not reconciled, and deliberately so.** The old census's
@@ -200,21 +200,21 @@ be stated is what this instrument counts, which is written above and re-runnable
200
200
at any commit.
201
201
202
202
Two structural facts do plausibly widen this reading against any hand or regex
203
-
one, and both are counted in the generated tables below: the 50 sites reached
203
+
one, and both are counted in the generated tables below: the 51 sites reached
204
204
through an erased (`any`) receiver, and the 50 that name their object through a
205
205
`const` rather than inline. An instrument that read either the way a person does
206
206
would report a smaller number and would not say so.
207
207
208
208
The fourth row is the one worth flagging to anyone citing it. **The 135 / 77%
209
209
figure has no surviving corroboration anywhere in the tree.** This census reads
210
-
125 of 236 (53%) as decidably elevated, with 103 more whose elevation is a
210
+
125 of 237 (53%) as decidably elevated, with 104 more whose elevation is a
211
211
run-time fact — so the claim is neither confirmed nor refuted, and the honest
212
212
answer is that a static reading cannot settle it.
213
213
214
-
⇒ **Cite `2 / 236`, and say what it is**: the sites whose options argument was
214
+
⇒ **Cite `2 / 237`, and say what it is**: the sites whose options argument was
215
215
READ and holds no tenant context, against a decidably tenancy-enabled object.
216
216
That is the control's provable yield surface. ⛔ Do not cite it as "the sites
217
-
without tenant context" — **31 further sites** have an options argument this
217
+
without tenant context" — **32 further sites** have an options argument this
218
218
cannot read, and they are neither in nor out.
219
219
220
220
{/* BEGIN GENERATED: tenant-audit-census (scripts/tenant-audit-census.mjs) — DO NOT EDIT */}
@@ -223,28 +223,28 @@ cannot read, and they are neither in nor out.
223
223
224
224
| what | count |
225
225
| :--- | ---: |
226
-
| write call sites on the application surface |**236**|
227
-
| …whose object name is statically decidable |156|
226
+
| write call sites on the application surface |**237**|
227
+
| …whose object name is statically decidable |157|
228
228
| …whose object name is chosen at run time | 80 |
229
-
| …against an object with tenancy ENABLED |155|
229
+
| …against an object with tenancy ENABLED |156|
230
230
| …against an object that declares tenancy off | 1 |
231
231
| threading a tenant context | 176 |
232
232
| PROVABLY carrying none (options read, no context key) |**8**|
233
233
| …of those, against a decidably tenancy-enabled object |**2**|
234
-
| options argument UNREADABLE — may or may not carry one |52|
235
-
| …of those, against a decidably tenancy-enabled object |31|
234
+
| options argument UNREADABLE — may or may not carry one |53|
235
+
| …of those, against a decidably tenancy-enabled object |32|
236
236
| threading a decidably ELEVATED (`isSystem`) context | 125 |
237
237
| threading a context that is decidably NOT elevated | 0 |
238
-
| threading a context whose elevation is a run-time fact |103|
238
+
| threading a context whose elevation is a run-time fact |104|
239
239
240
240
| how the instrument reached the site | count |
241
241
| :--- | ---: |
242
242
| receiver carried a readable engine type | 186 |
243
-
| receiver erased, placed by the object NAME |30|
243
+
| receiver erased, placed by the object NAME |31|
244
244
| receiver erased, placed by an `object: string` PARAMETER | 15 |
245
245
| receiver erased, placed by an `UNTYPED_RECEIVERS` row | 5 |
246
246
247
-
| object name spelled inline |106|
247
+
| object name spelled inline |107|
248
248
| object name spelled through a `const`| 50 |
249
249
| object name is an `object: string` parameter | 19 |
250
250
| object name is some other run-time expression | 61 |
@@ -297,13 +297,13 @@ holds still. They are required to be HERE and to say WHEN they were true;
297
297
their values are not compared. The reasoning, and the measurement behind it,
0 commit comments