Skip to content

Commit 4692c4f

Browse files
mason: fix cold-stall review correctness and writer fairness
Keep owner suspension at postprocess entry and replay committed reasoning/drop omissions before serving. Preserve coherent prepared/tagged input, avoid dropping synthetic history, and audit every carried snapshot. Build file-worker ranks from the complete current exclusion predicate; drain oversized memory append pages. Account for oversized sources in short identity/progress transactions and record their FTS omission, while keeping normal messages searchable. Normalize/hash outside writer transactions. Process-local foreground interest prevents new maintenance chunks; a colliding awaited turn waits for its owned chunk before its SQLite budget starts. Do not change shared persistent FTS settings. Unchanged independent review: all seven witnesses/controls pass, eight served passes byte-identical. Added peer-drop and real watchdog controls pass. Default-FTS 2M-row benchmark found native COMMIT max 1326.89ms, asynchronous foreground collision max 1328.51ms, p99 16.01ms; parent accepted this documented tail. 60MB skipped source: 231 chunks, max hold 0.972ms. Per-fix mutation proofs, snapshot audit and gates are in docs/reports/oc-cold-stall-corrections.md. Plugin has only the documented baseline WASM module-resolution failure; Pi, typecheck and lint pass. No witness expectations changed; no live stores accessed. Co-authored-by: Alfonso <alfonso-magic-context@users.noreply.github.com>
1 parent 5f25522 commit 4692c4f

13 files changed

Lines changed: 1037 additions & 81 deletions
Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
# Cold-stall correctness corrections
2+
3+
Follow-up to `9fd10c8085893746aca06b9d68f97473a118654b` and independent review
4+
`1107f2e00fc0ab7808e3c9ddd2385ada02f33477` (cherry-picked as `cfd9a89c9f`). The
5+
review report and witness expectations were not changed.
6+
7+
## Four fixes
8+
9+
1. **Durable omissions win.** The owner wait now runs at postprocess entry,
10+
before any local DB decision/protection snapshots or mutations. The former
11+
early transform owner wait is non-awaiting prewarm. Reasoning IDs and the
12+
drop-mode flag are reread as a committed union after later async todo/search
13+
work, even if no new IDs were selected. The current model epoch's cleared
14+
reasoning watermark is replayed too. Dropped-tag statuses are freshly replayed
15+
against the original target graph before final representation. Authorized
16+
latest-turn restoration is followed by omission replay again, so it cannot
17+
undo a peer's committed removal. Read failures fail closed, not to an empty set.
18+
A new real-WAL peer-drop test complements the unchanged peer-reasoning witness.
19+
2. **Seeds never certify exclusion.** Every rank-building row now evaluates the
20+
complete current summary/finish predicate inside the worker's read snapshot.
21+
Seeded candidate discovery remains only a hint; missing moved-in IDs cannot
22+
become ordinary rows by default. This also covers changed flags below the
23+
anchors. The tradeoff is that every worker re-rank may parse all session JSON;
24+
serving-thread counts remain bounded, exact or deferred, never full scans.
25+
3. **Short source accounting and process-local writer priority.** Normal source
26+
normalization/hashing moved outside the write transaction. Documents exceeding
27+
64 KiB normalized UTF-8 are omitted from FTS, with an explicit durable skip
28+
reason, original content hash, source identity and ordinal. Their identity is
29+
computed in 256 KiB byte chunks; each progress update is a separate short
30+
transaction, giving way between chunks. Final source/progress publication is
31+
another short transaction. Skip records are removed by explicit message/index
32+
cleanup; normal following messages remain searchable. UTF-8 encoding happens
33+
before chunk hashing, so a surrogate pair crossing a boundary does not alter
34+
the identity.
35+
36+
The process-local coordinator announces foreground interest before waiting.
37+
A worker obtains a permit for one chunk only when no foreground turn is active
38+
or waiting, rechecks after CAS, and releases it after COMMIT/rollback. A colliding
39+
awaited transform waits asynchronously for the in-flight permit **before** its
40+
250 ms SQLite acquisition lease starts. No new chunk can overtake it. Worker
41+
termination is awaited before releasing an orphaned permit. This is not a
42+
cross-process lock: SQLite continues to arbitrate other hosts. Shared FTS
43+
automerge/crisismerge/WAL policy was not changed.
44+
4. **Memory append pages are drained.** The memory path drains every 513-row page
45+
before advancing its global high watermark, rather than treating the first
46+
page as complete. File serving paths retain their bounded defer guard.
47+
48+
## Snapshot-to-apply audit
49+
50+
The removed late wait crossed the following local DB-derived views. Moving it to
51+
entry means all these reads/derivations now occur **after** that suspension:
52+
53+
- whole-part removal IDs, readable-state flag and drop-leaves-reasoning policy;
54+
- frozen merged/binding IDs, retained active thinking, protected thinking set,
55+
mutation-protected targets and the safe reasoning cutoff;
56+
- pending-operation list and its application/accounting decisions;
57+
- temporal decisions and their frozen first-application policy;
58+
- cached M0/M1 fallback capture, off-wire prefix preparation, hard-fold/ride and
59+
routine/emergency cleanup admission, and emergency episode observations;
60+
- protected tool IDs derived from active tags, and the applied cleanup result;
61+
- binding-prefix omission projection and calibration/owner estimates used in
62+
fresh reasoning selection.
63+
64+
Todo synthesis and auto-search still contain existing later awaits. A fresh
65+
committed omission union and fresh visible dropped statuses are read after them,
66+
independently of local selection/persistence. Final merged/binding decisions come
67+
from the existing later replay snapshot; whole-part IDs are explicitly unioned
68+
rather than replaced. Cleared watermark replay is monotone only within the same
69+
`lastObservedModelKey`, preserving the deliberate model-transition reset.
70+
71+
The following **parent-supplied pass inputs** cross entry's wait. They are retained
72+
as one coherent tagged/prepared input, per the review ruling; individual pieces
73+
are not rebound to a newer peer render:
74+
75+
| Input | Why it is carried coherently / post-wait treatment |
76+
|---|---|
77+
| Tagged message graph, message-tag numbers, target closures and captured pristine source bodies | Already created and used together by tagging; rebuilding only one map would point edits at different part coordinates. Durable drops replay through this same graph. |
78+
| Partial-compartment bounds embedded in target closures | Part of the same original target coordinate system; not separately refreshed underneath closures. |
79+
| Active tag/protected-token window, protected tag IDs/numbers, cutoff and count | One caller-computed protection cohort for these wire objects; not replaced with a peer's tag mass while retaining old targets. Fresh pending-op application already loads selected rows in its own transaction. |
80+
| Session metadata: cached M0/M1 bytes, mural, cached coverage/marker bounds, TTL/system identity, last todo state | Rendering/admission input associated with the prepared prefix. No wholesale metadata replacement discards local successful writes or relabels cached bytes. Monotone omission fields are reread separately before serving. |
81+
| Prepared compartment injection and its trim/coverage boundary; local history-rebuild outcomes | Bytes and seam/coverage are one tuple. Updating only its boundary would bless an older render as new history. |
82+
| Caller usage/overflow sample, window/limit geometry and derived scheduler/budget gates | These define this request's admission/protection decisions, not a newer concurrent request's measurements. |
83+
| Project binding / prefix-freeze verdict and host-vs-MC compaction classification | Bound to this host window and prepared-history identity. |
84+
| Thinking-recovery authorization and captured latest-turn originals | Bound to the caller's anchored turn. Restoration cannot override the final durable omission union. |
85+
| Tool-reclaim watermark and dropped-tag watermark used for new selections | Pass inputs; later selectors still use their existing transactional/fresh-row rules. Final visible drops/removals override older selection observations. |
86+
| Frozen tool availability, permission verdicts and pass-local publication/materialization sets | Intentional request policy, not reread as an unrelated peer's request. |
87+
88+
There is no owner suspension between these local reads and their application now.
89+
The former transform-level wait crossed session metadata/usage/overflow, recovery
90+
and project authorization, compaction classification and the tag-load floor; it
91+
has been removed entirely rather than selectively refreshing those derived chains.
92+
No broad epoch retries were added. The parent explicitly chose preservation of
93+
coherent pass input plus fresh durable omissions, not refusal under ordinary peers.
94+
95+
## Default-FTS measurement and accepted latency limit
96+
97+
Linux 8c, Bun 1.4.2. `index-chunk-hold.ts` builds **2,000,000 documents**, each with
98+
**2,560-byte** text, using default FTS settings throughout. It then measures 512
99+
incremental writes and foreground collisions on that same index. The config
100+
before/after contained only `{k:"version",v:4}`; no merge setting override occurred.
101+
`lsof` for host PID 2 showed only throwaway `context.db`, WAL and SHM paths under
102+
`$TMPDIR/magic-context/bg_c399011e1ec6641d-hold/`; all six isolation variables and
103+
SQLite temp were placed there. No live store/config was read or migrated.
104+
105+
Milliseconds:
106+
107+
| Operation | p50 | p95 | p99 | maximum |
108+
|---|---:|---:|---:|---:|
109+
| One normal incremental transaction hold | 0.673 | 0.987 | 14.371 | **1327.307** |
110+
| Native COMMIT | 0.167 | 0.314 | 13.878 | **1326.890** |
111+
| Foreground collision wait + its one-row write | 2.269 | 3.379 | 16.008 | **1328.511** |
112+
| 60 MB source's identity/skip chunk hold (231 transactions) | 0.007 | 0.029 | 0.205 | **0.972** |
113+
| 60 MB source's chunk COMMIT | 0.005 | 0.014 | 0.025 | **0.356** |
114+
115+
The default-policy rare merge is not bounded by document size. The benchmark
116+
stopped with `escalationRequired=true` / exit 2 because COMMIT exceeded one second.
117+
The parent reviewed these exact numbers and accepted this slice's **1.33 s maximum
118+
asynchronous collision wait**, with no global FTS-policy change: formerly the
119+
same native merge froze the main thread; now other sessions/event-loop callbacks
120+
remain responsive. This is a measured distribution, not a hard sub-1s promise.
121+
The setup's 500-document transactions had maximum 1753.115 ms; they are bulk fixture
122+
construction, not the incremental maintenance path.
123+
124+
Cross-process contention is still SQLite contention. This change improves the
125+
large-message case (no giant FTS insertion, sub-ms skip chunk holds) and gives
126+
way between chunks, but cannot give foreground priority to another Pi/OpenCode
127+
process. Normal default-policy merges may still hold the shared writer lock for
128+
the measured tail; off-thread execution changes responsiveness, not that native
129+
cross-process lock duration. Released hosts keep their default merge policy.
130+
131+
The real host profiler detects watchdog **lateness**, not request elapsed time:
132+
Bun and Node compare the tick gap against the 250 ms watchdog interval. A new
133+
real-timer test holds a coordination permit asynchronously for 1330 ms, waits
134+
through the foreground gate, observes over 100 event-loop ticks with no 500 ms
135+
stall report, then verifies the same watchdog reports a real 900 ms synchronous
136+
block. Inspector data alone is stubbed; timers, coordination and blocking are real.
137+
Thus the asynchronous collision wait is not counted as a main-thread stall.
138+
139+
## Gates and regression controls
140+
141+
All verification ran on Linux 8c. Versions: Bun 1.4.2, TypeScript 5.9.3,
142+
Biome 2.5.1. The enabled unchanged review passed all seven tests, including the
143+
full eight-pass byte differential (8123, 8123, 8123, 8123, 8123, 8145, 8145,
144+
8145 bytes), plus all its positive controls. Four additional correction tests
145+
passed, including a peer drop and the real-timer stall-profiler control.
146+
147+
Root build passed (708 / 750 / 1012 / 412 bundled modules for OpenCode/v2/Pi/CLI;
148+
nine TUI outputs unchanged). Full plugin suite: **7683 passed, 27 skipped, one
149+
known baseline failure**, 7711 tests / 777 files. The unchanged Node WASM
150+
Transformers fixture cannot resolve `onnxruntime-web/webgpu` from its temporary
151+
bundle on the remote runner; this is the same documented pre-existing failure,
152+
not a green full-suite claim. All affected tests and worker packaging tests pass.
153+
154+
Full Pi suite finally passed **1921 tests, ten skipped, zero failed**, 1931 tests
155+
across 196 files. A preceding loaded run had one timing flake: the unmodified slow
156+
embedding deadline test observed 3201.7 ms against its 3150 ms ceiling; a subsequent
157+
full suite passed unchanged. Frozen installs checked 1013 installs / 1254 packages,
158+
no dependency or lock changes. No suite exported `OPENCODE_DB` globally.
159+
160+
Four-package typecheck passed, as did a temporary test-only configuration checking
161+
the unchanged review and new correction test (the standard config excludes tests).
162+
Root lint passed after formatting, 1811 files across four packages, existing
163+
warnings only. The temporary configuration and baseline extraction are disposable
164+
verification inputs, not delivery files. Initial regressions in synthetic-history
165+
replay, statement-compilation bounds and the transaction-acquisition fence were
166+
fixed without rewriting their expectations; their focused rerun passed 19 tests.
167+
Visible-drop replay excludes synthetic/replaced history objects, and skip metadata
168+
uses per-connection cached statements.
169+
170+
## Mutation evidence
171+
172+
Live implementation files were staged before every temporary control. Every
173+
mutation produced a non-empty `git diff --stat`; restoration used the staged
174+
index plus `touch`, and an empty working diff was captured each time. No witness
175+
expectation was edited. Selected checks ran one exact test name, so unrelated
176+
checks were filtered, not falsely described as passing.
177+
178+
- Restoring the pre-wait removal snapshot and disabling its final union made the
179+
unchanged peer-reasoning witness red: private reasoning returned on the wire.
180+
- Restoring seeded-ID-only exclusion made the unchanged large-session move
181+
witness red: all three ordinals were 1025 instead of 1024.
182+
- Disabling the memory page drain made its unchanged witness red: 514 instead
183+
of 601 owners.
184+
- Bypassing coordination and admitting a giant document **while retaining
185+
outside-lock normalization** did not redden the refused-turn witness on this
186+
runner. This control is reported as undefended, not as a proof. Reintroducing
187+
normalization/hashing under the writer as well reproduced `SQLITE_BUSY` and
188+
reddened exactly that refused-turn witness. This distinguishes independent
189+
layers of protection instead of assuming every neutralization recreates a
190+
sufficiently long overlap.
191+
- Independently bypassing foreground coordination reddened its dedicated
192+
owned-chunk wait test; independently lifting the FTS size cap reddened the
193+
oversized-skip/normal-searchability test. These red controls reach both files
194+
involved in the undefended partial control.
195+
- Disabling final visible-drop replay reddened the new peer-drop witness.
196+
197+
After all restorations, the unchanged review, new correction controls and both
198+
SQLite scope/acquisition regression files passed: **28 tests, zero failures,
199+
123 assertions**. The eight-pass differential again matched every byte. Product
200+
and two test-only TypeScript checks then passed with TypeScript 5.9.3. The baseline
201+
extraction and temporary typecheck configuration were removed before commit.
202+
203+
Three sidekick comment-review attempts returned incomplete/truncated coverage
204+
instead of actionable findings; no clean comment-review result is claimed. The
205+
snapshot inventory was separately researched with concrete source citations and
206+
reviewed against the implementation. Comments/prose were manually checked for
207+
plain-language reasons and for no assertion that default-policy COMMIT is bounded
208+
below one second.

0 commit comments

Comments
 (0)