Skip to content

Commit 30cd950

Browse files
ARHAEEMclaude
andcommitted
fix(sync,daemon,lsp): prune data-loss guards, force-kill identity, quadratic diagnostics
Six medium findings from the audit. Each is pinned by a regression test. sync/records.js: pruneRecords' only "don't prune after a failure" gate was RUN-wide, so one converged row in any other table (counted as skipped) disarmed it. A FIELD_FORBIDDEN 403 is a per-field permission property that fails every row of exactly one table by design, so a wholly-failed table is routine -- and under mirror+confirmDeletions its pre-existing dest rows were deleted while the replacement data was never written, reporting phase=done. Pass 1 now records per-table outcomes and prune skips a table that wrote nothing and failed, emitting RECORDS_FAILED_PRUNE_SKIPPED. sync/prune-schema.js: pruneSchema deleted the fieldMappings targets apply() had just validated. A target must exist on the dest but not on the source -- the documented injection pattern (source autoNumber Code -> dest text field InjectID) is dest-only by construction and so always an orphan. The records job then re-validated, threw FIELD_MAP_INVALID and synced zero records: a destination column destroyed and no data written. sync/index.js: the drift guard fingerprints only the DESTINATION, but plan.orphans is a statement about the SOURCE. A source-side addition between plan and apply was invisible -- dest untouched so the fingerprint matched, no DRIFT, no create action in the stale plan -- and pruneSchema then deleted the dest field that now legitimately matched. expectedName cannot help; the field kept its name. apply() now re-reads the source before any schema deletion and drops orphans that regained a counterpart; if that read fails it skips all schema deletion rather than trusting a stale list. daemon/launcher.js: stopDaemon --force SIGKILLed a lockfile pid with no identity check. isStale()'s bare process.kill(pid,0) cannot tell our daemon from a process that recycled the pid, and on Windows SIGTERM maps to TerminateProcess -- an unsavable hard kill of a stranger. Now probes /daemon/health and requires the uuid to echo the lockfile's, mirroring the extension's _verifyDaemonIdentity. Unproven means release the lock and leave the process alone. sync/records.js: buildUpdateCells never compared against the destination, so every mapped scalar cell of every mapped row was re-posted on each re-sync -- updateRecords issues one serialized POST per CELL, i.e. 20,000 sequential requests for a 1000-row x 20-field no-op run. Converged cells are now skipped. language-services diagnostics.ts: isInsideExclusionRange was a linear scan run once per character by three checkers, so cost was chars x field-refs with no debounce and no size cap -- and in --tcp daemon mode that blocking is shared by every attached editor. Now a binary search over the already-ascending ranges. Measured on the repo's largest shipped example (38,830 chars / 741 refs): 83.3ms -> 9.6ms. On an 87KB/4000-ref synthetic: 592ms -> 10ms. Diagnostic output identical. Verified: 1451 mcp-server + 162 language-services + 88 webview tests pass, check:tool-sync green, pnpm build succeeds. One extension test (session-backup "rejects backups larger than 200 MB") fails with ENOSPC -- the disk is 100% full and that test truncates a 201 MB file. It passed earlier this session and neither it nor packages/extension is touched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6bb81a8 commit 30cd950

8 files changed

Lines changed: 434 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,19 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
66

77
## [Unreleased]
88

9+
### Fixed — formula diagnostics were quadratic, blocking the editor on every keystroke (2026-07-31)
10+
11+
- **`isInsideExclusionRange` was a linear scan run once per character.** `ranges.some(...)` is
12+
O(field refs) and is called per character by `checkParentheses`, `checkQuotes` and
13+
`checkBrackets`, plus once per match by five more checkers — so cost was chars × refs, with no
14+
debounce and no size cap on either entry point (`registration.ts`'s `onDidChangeTextDocument`
15+
and the LSP's `onDidChangeContent`, which in `--tcp` daemon mode is shared by every attached
16+
editor). `getFieldRefRanges` emits ascending, non-overlapping spans, so this is now a binary
17+
search. Measured on this repo's own largest shipped example
18+
(`examples/[IGD-JSON]~[Payload]~[Formula].formula`, 38,830 chars / 741 refs):
19+
**83.3 ms → 9.6 ms** per run. On an 87 KB / 4,000-ref synthetic: **592 ms → 10 ms**. Diagnostic
20+
output is byte-identical before and after.
21+
922
### Fixed — Unconfigure destroyed unrelated config in Codex / Helix files (2026-07-31)
1023

1124
- **`unconfigureMcpToml` and `unconfigureHelix` truncated the user's config file from our marker

packages/language-services/src/engines/formula/diagnostics.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,35 @@ function getExclusionRanges(text: string): Array<{ start: number; end: number }>
120120
return ranges;
121121
}
122122

123+
/**
124+
* Is `position` inside one of the excluded spans?
125+
*
126+
* Called once per CHARACTER by checkParentheses, checkQuotes and checkBrackets, and
127+
* once per match by five more checkers. `ranges.some(...)` made that chars × refs —
128+
* on this repo's own largest shipped example (38,830 chars / 741 field refs) that is
129+
* ~29M comparisons of blocked extension host per keystroke, with no debounce and no
130+
* size cap on either entry point (registration.ts's onDidChangeTextDocument and the
131+
* LSP's onDidChangeContent, which in --tcp mode is shared by every attached editor).
132+
*
133+
* getFieldRefRanges scans left to right and does not nest, so `ranges` is already
134+
* sorted and non-overlapping — binary search is a drop-in, allocation-free swap.
135+
*/
123136
function isInsideExclusionRange(
124137
position: number,
125-
ranges: Array<{ start: number; end: number }>
138+
ranges: Array<{ start: number; end: number }>,
139+
startPad = 0,
140+
endPad = 0
126141
): boolean {
127-
return ranges.some(range => position >= range.start && position < range.end);
142+
let lo = 0;
143+
let hi = ranges.length - 1;
144+
while (lo <= hi) {
145+
const mid = (lo + hi) >> 1;
146+
const range = ranges[mid];
147+
if (position < range.start + startPad) hi = mid - 1;
148+
else if (position >= range.end + endPad) lo = mid + 1;
149+
else return true;
150+
}
151+
return false;
128152
}
129153

130154
// Spans of complete {…} field references, computed outside string literals (no nesting).
@@ -302,7 +326,11 @@ function checkBrackets(text: string, uri?: string): LsDiagnostic[] {
302326
for (let i = 0; i < text.length; i++) {
303327
// Skip the INTERIOR of complete {…} refs (a quote in a field name must not flip
304328
// string state) but still see the braces themselves so balance-checking works.
305-
const interior = fieldRefRanges.some(r => i > r.start && i < r.end - 1);
329+
// Binary search, not .some() — this runs once per character (see the note on
330+
// isInsideExclusionRange). Shrinking the span by one at each end reproduces the
331+
// old strict `i > r.start && i < r.end - 1` predicate: the braces stay visible so
332+
// balance-checking still works, only the field NAME is skipped.
333+
const interior = isInsideExclusionRange(i, fieldRefRanges, 1, -1);
306334
if (interior) continue;
307335

308336
if (

packages/mcp-server/CHANGELOG.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,52 @@
22

33
## [Unreleased]
44

5+
### Fixed (2026-07-31 — sync prune could delete data it had not replaced)
6+
7+
- **`pruneRecords` had only a RUN-wide failure gate.** The "don't prune after a failed run" check
8+
was `failed > 0 && created === 0 && updated === 0 && skipped === 0` against global accumulators,
9+
so a single converged row in *any other table* (counted as `skipped`) disarmed it. A
10+
`FIELD_FORBIDDEN` 403 is a per-field permission property that fails every row of exactly one
11+
table by design, so "one table wholly failed, the rest fine" is routine — and under
12+
`mirror` + `confirmDeletions` that table's pre-existing dest rows were deleted while their
13+
replacement data was never written, with the job reporting `phase='done'`. Pass 1 now records
14+
per-table outcomes (`result.perTable`) and `pruneRecords` skips any table with
15+
`failed > 0 && created + updated + skipped === 0`, emitting `RECORDS_FAILED_PRUNE_SKIPPED`
16+
the same shape as the existing truncation guard.
17+
- **`pruneSchema` deleted the `fieldMappings` targets `apply()` had just validated.** A mapping
18+
target must exist on the dest and be writable but is *not* required to exist on the source — the
19+
documented injection pattern (source autoNumber `Code` → dest text field `InjectID`) is dest-only
20+
by construction and therefore always classified an orphan. `apply()` validated the mappings, then
21+
called `pruneSchema` without them and deleted the target; the background records job then
22+
re-validated, threw `FIELD_MAP_INVALID` and synced zero records. Net effect: a destination column
23+
destroyed and no data written. `fieldMappings` is now threaded through and targets are kept with
24+
`FIELD_MAP_TARGET_PROTECTED`.
25+
- **The drift guard fingerprints only the DESTINATION, but `plan.orphans` is a statement about the
26+
SOURCE.** A source-side *addition* between plan and apply was invisible: the dest was untouched so
27+
the fingerprint matched, no `DRIFT`/`RESUME_DRIFT` fired, `applyPlan` had no create action for the
28+
new field (the plan predates it) — and `pruneSchema` then deleted the dest field that now
29+
legitimately matched, taking its data with it. `deleteField`'s `expectedName` guard cannot help,
30+
because the dest field kept its name. `apply()` now re-reads the source before any schema deletion
31+
and drops orphans that have regained a counterpart (`ORPHAN_STALE_SKIPPED`); if that read fails it
32+
skips all schema deletion for the run rather than trusting a stale list (`ORPHAN_RECHECK_FAILED`).
33+
The extra read is only paid when deletions are actually confirmed.
34+
35+
### Fixed (2026-07-31 — force-stop could kill an unrelated process; converged re-syncs re-posted every cell)
36+
37+
- **`stopDaemon --force` SIGKILLed a pid read from the lockfile with no identity check.** The only
38+
thing establishing that the pid was the daemon was `isStale()`'s bare `process.kill(pid, 0)`
39+
liveness probe, which cannot distinguish our daemon from a process that recycled the pid after an
40+
unclean death. In that state the lock persists with a live-but-foreign pid, the 10s wait never
41+
clears, and the force branch signalled a stranger — on Windows `SIGTERM` maps to
42+
`TerminateProcess`, so even the "graceful" first step is an unsavable hard kill. Now mirrors the
43+
extension's `_verifyDaemonIdentity`: probe `/daemon/health` and require the uuid to echo the
44+
lockfile's. Unproven ⇒ release the stale lock, leave the process alone, and say so.
45+
- **`buildUpdateCells` performed no comparison against the destination.** `updateRecords` issues one
46+
serialized HTTP POST *per cell*, and every mapped writable scalar was re-posted on every re-sync
47+
even when nothing had changed — 1000 rows × 20 fields = 20,000 sequential requests for a run that
48+
changes nothing. Converged cells are now skipped, which collapses most rows to zero cells and lets
49+
the existing empty-row skip drop them entirely. Cleared-cell propagation is unaffected.
50+
551
### Fixed (2026-07-31 — tunnel revocation reached only half the cases)
652

753
- **The daemon held TWO independent `activeTunnel` handles**, one in `daemon/server.js` (filled by

packages/mcp-server/src/daemon/launcher.js

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,36 @@ export async function stopDaemon(options = {}) {
561561
}
562562

563563
const pid = recordForShutdown.pid;
564+
565+
// NEVER signal a pid we have not PROVEN is our daemon.
566+
//
567+
// isStale()'s liveness probe is a bare `process.kill(pid, 0)` — it cannot tell our
568+
// daemon from an unrelated process that recycled the pid after an unclean death.
569+
// In that state the lock persists with a live-but-foreign pid, so status is
570+
// running/unhealthy, the 10s wait never clears, and this branch used to SIGTERM
571+
// then SIGKILL a stranger's process. On Windows `process.kill(pid,'SIGTERM')` maps
572+
// to TerminateProcess, so even the "graceful" step is an unsavable hard kill.
573+
// The extension already gates its equivalent on _verifyDaemonIdentity; this
574+
// mirrors it — probe /daemon/health and require the uuid to echo the lockfile's.
575+
let provenOurDaemon = false;
576+
try {
577+
const health = await adminRequest(recordForShutdown, '/daemon/health', { method: 'GET' });
578+
provenOurDaemon = !!recordForShutdown.uuid && health?.uuid === recordForShutdown.uuid;
579+
} catch { /* unreachable → unproven, fall through */ }
580+
581+
if (!provenOurDaemon) {
582+
// Release the lock so a fresh daemon can start, but leave the process alone.
583+
try {
584+
release({ lockPath: getLockfilePath(configDir), expectedUuid: recordForShutdown.uuid });
585+
} catch { /* best-effort */ }
586+
return {
587+
stopped: false,
588+
forced: false,
589+
pid,
590+
reason: `Refusing to force-kill pid ${pid}: it did not answer /daemon/health with this lockfile's uuid, so it cannot be shown to be our daemon — the pid may have been recycled by an unrelated process. The stale lock was released, so a new daemon can start normally. If you are certain pid ${pid} is a hung airtable-user-mcp daemon, end it yourself.`,
591+
};
592+
}
593+
564594
let signalled = false;
565595
try {
566596
process.kill(pid, 'SIGTERM');

packages/mcp-server/src/sync/index.js

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,52 @@ export async function apply({ client, sourceBaseId, destBaseId, planId, runStart
335335
// applyPlan so matched fields/views exist (orphan deps safe) and BEFORE the records job so
336336
// the schema is clean before record sync begins. Mutates `result` in-place.
337337
if (!result.aborted) {
338-
await pruneSchema({ client, destAppId: destBaseId, plan: fullPlan, policy, policyOverrides, confirmDeletions, confirmTableDeletions, result });
338+
// Re-validate the orphan list against the LIVE source before deleting anything.
339+
//
340+
// The drift guard above fingerprints only the DESTINATION, but `plan.orphans` is a
341+
// statement about the SOURCE ("this dest object has no source counterpart"). A
342+
// source-side ADDITION between plan and apply is therefore completely invisible:
343+
// the dest is untouched so the fingerprint matches, no DRIFT fires, applyPlan has
344+
// no create action for the new field (the plan predates it) — and pruneSchema then
345+
// deletes the dest field that now legitimately matches, taking its data with it.
346+
// deleteField's expectedName guard cannot help: the dest field kept its name.
347+
//
348+
// Only pay for the extra read when we are actually going to delete something.
349+
let prunePlan = fullPlan;
350+
const orphanList = fullPlan.orphans || [];
351+
if (orphanList.length && (confirmDeletions || confirmTableDeletions)) {
352+
try {
353+
const srcNow = await snapshotSchemaOnly(client, sourceBaseId);
354+
const srcTablesByName = new Map((srcNow.tables || []).map((t) => [t.name, t]));
355+
const stillOrphaned = orphanList.filter((o) => {
356+
if (o.kind === 'table') return !srcTablesByName.has(o.name);
357+
const st = srcTablesByName.get(o.tableName);
358+
if (!st) return true; // whole source table gone → its children are still orphans
359+
const pool = o.kind === 'field' ? (st.fields || [])
360+
: o.kind === 'view' ? (st.views || [])
361+
: (st.sections || []);
362+
return !pool.some((x) => x.name === o.name);
363+
});
364+
for (const o of orphanList.filter((o) => !stillOrphaned.includes(o))) {
365+
result.warnings.push({
366+
code: 'ORPHAN_STALE_SKIPPED',
367+
message: `${o.kind} "${o.name}"${o.tableName ? ` in "${o.tableName}"` : ''} was planned for deletion but now has a source counterpart — the source changed after plan ${planId}. Kept. Re-run mode=plan to sync it.`,
368+
});
369+
}
370+
if (stillOrphaned.length !== orphanList.length) prunePlan = { ...fullPlan, orphans: stillOrphaned };
371+
} catch (e) {
372+
// Cannot prove the orphan list is current → do not delete on a stale one.
373+
prunePlan = { ...fullPlan, orphans: [] };
374+
result.warnings.push({
375+
code: 'ORPHAN_RECHECK_FAILED',
376+
message: `Could not re-read the source to confirm ${orphanList.length} planned deletion(s) are still orphans (${e.message ?? e}); skipped all schema deletions this run.`,
377+
});
378+
}
379+
}
380+
381+
// fieldMappings is threaded through so pruneSchema never deletes a mapping
382+
// TARGET — those are dest-only by construction and therefore always orphans.
383+
await pruneSchema({ client, destAppId: destBaseId, plan: prunePlan, policy, policyOverrides, confirmDeletions, confirmTableDeletions, result, fieldMappings });
339384

340385
// Records phase: runs after schema apply, only if not aborted. It is minutes-long for large
341386
// bases, so we launch it in the BACKGROUND (fire-and-forget) and return immediately — a single
1.32 KB
Binary file not shown.

packages/mcp-server/src/sync/records.js

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,11 +349,41 @@ export function buildUpdateCells(srcFields, srcCells, destCells, idmap, warnings
349349
continue;
350350
}
351351
if (Array.isArray(coerced.value)) continue; // defensive: no array must ever enter the row payload
352+
// Skip cells the destination already agrees with. updateRecords issues ONE
353+
// serialized HTTP POST per CELL (client.js `updatePrimitiveCell`), and without
354+
// this comparison every mapped scalar cell of every mapped row was re-posted on
355+
// every re-sync even when nothing had changed — 1000 rows x 20 fields = 20,000
356+
// sequential requests for a no-op run. On a converged table this collapses most
357+
// rows to zero cells, which the existing empty-row skip then drops entirely.
358+
if (destCells && sameCellValue(destCells[mapping.destFld], coerced.value)) continue;
352359
cells[mapping.destFld] = coerced.value;
353360
}
354361
return cells;
355362
}
356363

364+
/**
365+
* Value equality for the Pass-1 update diff.
366+
*
367+
* Only ever sees scalars here — array-shaped cells returned earlier, and the
368+
* Array.isArray guard above is belt-and-braces — so strict equality covers the
369+
* common case and a structural compare covers the object-shaped ones (select
370+
* choices, dates carrying a timezone wrapper). Deliberately conservative: when in
371+
* doubt it reports "different" and we simply write the cell as before.
372+
*/
373+
function sameCellValue(a, b) {
374+
if (a === b) return true;
375+
// An absent dest cell and an explicit null/'' source cell are not worth a request
376+
// apart, but only treat them equal when BOTH sides are empty-ish.
377+
const empty = (v) => v === undefined || v === null || v === '';
378+
if (empty(a) && empty(b)) return true;
379+
if (a === null || b === null || typeof a !== 'object' || typeof b !== 'object') return false;
380+
try {
381+
return JSON.stringify(a) === JSON.stringify(b);
382+
} catch {
383+
return false;
384+
}
385+
}
386+
357387
/**
358388
* Pass 1 record sync: scalar/select upsert + fill the global record map.
359389
*
@@ -558,6 +588,17 @@ export async function applyRecordsPass1({ client, srcSnapshot, destSnapshot, idm
558588
for (const srcTable of orderedTables) {
559589
// A session death latched by a prior table/chunk aborts the whole job — never start a new table.
560590
if (client.auth?.isSessionDead?.()) { markAborted(client, result); return; }
591+
// Per-table write outcome, derived by diffing the run-wide counters across this
592+
// iteration. pruneRecords needs it: the only "don't prune after a failure" gate
593+
// used to be RUN-wide (failed>0 && created===0 && updated===0 && skipped===0), so a
594+
// single converged row anywhere in the run disarmed it — and a table whose every
595+
// write failed (a FIELD_FORBIDDEN 403 is a schema-level property that fails every
596+
// row of one table by design) then had its pre-existing dest rows deleted under
597+
// mirror while the replacement data was never written.
598+
const countsBefore = {
599+
created: result.created, updated: result.updated,
600+
skipped: result.skipped, failed: result.failed,
601+
};
561602
const destTableId = idmap.tables[srcTable.id];
562603
if (!destTableId) continue; // table not matched → skip
563604

@@ -711,6 +752,16 @@ export async function applyRecordsPass1({ client, srcSnapshot, destSnapshot, idm
711752
}
712753
persist(idmap, journal);
713754
}
755+
756+
// Record what this table actually achieved, for pruneRecords' per-table gate.
757+
// Tables that `continue` above never reach here — they attempted no writes, so
758+
// the absence of an entry correctly reads as "nothing failed".
759+
(result.perTable ??= {})[srcTable.name] = {
760+
created: result.created - countsBefore.created,
761+
updated: result.updated - countsBefore.updated,
762+
skipped: result.skipped - countsBefore.skipped,
763+
failed: result.failed - countsBefore.failed,
764+
};
714765
}
715766
}
716767

@@ -1706,6 +1757,24 @@ export async function pruneRecords({ client, destSnapshot, idmap, policy, policy
17061757
continue;
17071758
}
17081759

1760+
// Per-table write-failure gate. Deleting dest-only rows in a table whose every
1761+
// write failed destroys the existing data AND leaves nothing in its place — the
1762+
// run-wide RECORDS_ALL_FAILED gate cannot catch it, because one converged row in
1763+
// any other table (counted as `skipped`) disarms it. A FIELD_FORBIDDEN 403 is a
1764+
// per-field permission property, so "every row of exactly one table failed" is a
1765+
// routine outcome, not an exotic one.
1766+
const tableCounts = result.perTable?.[t.name];
1767+
if (tableCounts && tableCounts.failed > 0
1768+
&& tableCounts.created === 0 && tableCounts.updated === 0 && tableCounts.skipped === 0) {
1769+
result.warnings.push({
1770+
code: 'RECORDS_FAILED_PRUNE_SKIPPED',
1771+
message: `Table "${t.name}": all ${tableCounts.failed} record write(s) failed — skipping mirror ` +
1772+
`deletion so dest-only rows are not destroyed while their replacement data was never written. ` +
1773+
`Fix the cause (see warnings) and re-run mode=apply with the same planId.`,
1774+
});
1775+
continue;
1776+
}
1777+
17091778
// Prefer the first collaborative (non-personal) view; fall back to first view of any type.
17101779
const views = t.views || [];
17111780
const viewId = views.find((v) => !v.personalForUserId)?.id ?? views[0]?.id;

0 commit comments

Comments
 (0)