fix(app): hold input until the schema-cache refresh lands (#39)
A simple-mode Form-B insert (`insert into T values (…)`) submitted faster than the post-DDL schema-cache refresh was validated against the stale cache and wrongly rejected as "trying to write SQL?". The cache is refreshed asynchronously after the worker applies a command, posting `SchemaCacheRefreshed` on the same FIFO channel as key events; under faster-than-human input the next Enter is processed before the refresh. Add a submission gate: `dispatch_dsl` arms `awaiting_schema_refresh` on every `ExecuteDsl` dispatch, holds further DSL submissions in a `held_submissions` FIFO while armed, and the `SchemaCacheRefreshed` handler drains them against the fresh cache in submission order. App-lifecycle commands bypass the gate. Arm-on-every-dispatch keeps exactly one DSL command in flight, so the boolean is provably correct. Interactive-only: the replay/batch path already re-snapshots the schema synchronously per line. No interactive-user impact; held input is never lost (a refresh always follows a dispatch). Tests (both verified red→green): a Tier-1 deterministic ordering test and a Tier-4 PTY back-to-back regression (the unpaced inverse of flow 3). Records the design as ADR-0022 Amendment 8.
This commit is contained in:
@@ -824,6 +824,63 @@ of issue #4; no `AmbientHint` / renderer change. Covered by
|
|||||||
seed_count_hint_does_not_leak_once_the_count_or_a_clause_is_given,
|
seed_count_hint_does_not_leak_once_the_count_or_a_clause_is_given,
|
||||||
seed_count_hint_also_fires_after_a_column_fill_target}`.
|
seed_count_hint_also_fires_after_a_column_fill_target}`.
|
||||||
|
|
||||||
|
## Amendment 8 — submission gate: hold input until the schema-cache refresh lands (2026-06-22)
|
||||||
|
|
||||||
|
§9 established the schema cache as the source of truth for the
|
||||||
|
walker's schema-aware dispatch, "refreshed by the runtime … after
|
||||||
|
successful DDL". That refresh is **asynchronous**: the runtime applies
|
||||||
|
a command on the worker thread (ADR-0010) and only afterwards posts a
|
||||||
|
`SchemaCacheRefreshed` event back through the **same FIFO channel that
|
||||||
|
carries key events**. Between dispatching a DDL command and that event
|
||||||
|
landing, `schema_cache` is stale w.r.t. the command just run.
|
||||||
|
|
||||||
|
Validation in `App::update` is pure-sync (a core invariant — it cannot
|
||||||
|
do a DB round-trip), so it can only consult that cache. Under
|
||||||
|
**faster-than-human input** (paste, scripted input, an unpaced PTY
|
||||||
|
driver), the next submission's Enter is already queued *ahead* of the
|
||||||
|
refresh event, so it is validated against the stale cache. A simple-mode
|
||||||
|
**Form-B insert** (`insert into T values (…)`, columns derived from the
|
||||||
|
cache) submitted right after `add column` then sees the pre-DDL columns,
|
||||||
|
its value arity can't match, and the friendly layer tags it *"trying to
|
||||||
|
write SQL?"* — even though the identical line succeeds at human speed
|
||||||
|
(issue **#39**).
|
||||||
|
|
||||||
|
**Decision — gate submission on the pending refresh.** `App` carries
|
||||||
|
`awaiting_schema_refresh: bool` + a `held_submissions` FIFO queue.
|
||||||
|
`dispatch_dsl` arms the flag on **every** `ExecuteDsl` dispatch; while
|
||||||
|
armed, a new DSL submission is **held** (queued in submission order)
|
||||||
|
rather than validated. The `SchemaCacheRefreshed` handler clears the
|
||||||
|
flag and **drains** the queue against the now-fresh cache, stopping as
|
||||||
|
soon as a drained command re-arms the gate (so the remainder wait for
|
||||||
|
*its* refresh — order preserved). A held command that doesn't dispatch
|
||||||
|
(parse error / pre-flight rejection) leaves the gate open and the loop
|
||||||
|
continues. App-lifecycle commands (`quit` / `help` / `load` / `undo` /
|
||||||
|
`rebuild`) route through `dispatch_app_command` *before* `dispatch_dsl`
|
||||||
|
and so are never held.
|
||||||
|
|
||||||
|
**Why arm on every dispatch, not just DDL.** It keeps at most one DSL
|
||||||
|
command in flight, so refreshes are strictly one-per-dispatch and in
|
||||||
|
order — making the gate a provably-correct boolean. Arming only on
|
||||||
|
schema-mutating commands would let a *preceding* non-DDL command's
|
||||||
|
refresh clear the gate early and drain a held Form-B insert against a
|
||||||
|
cache that predates the DDL — reintroducing the bug in a corner case.
|
||||||
|
(App-lifecycle commands also refresh but bypass the gate; they are
|
||||||
|
modal/picker-gated and so cannot overlap a rapid DSL paste, the only
|
||||||
|
thing this guards.)
|
||||||
|
|
||||||
|
**Scope.** Interactive input only. The `replay` / history-log / startup
|
||||||
|
rebuild-from-text batch path already re-snapshots the schema
|
||||||
|
**synchronously, inline, before every line** (`run_replay`,
|
||||||
|
`build_schema_cache`), so it has always had this ordering guarantee;
|
||||||
|
this amendment brings the interactive path in line with it. No
|
||||||
|
interactive-user impact (the gate clears in milliseconds); held input is
|
||||||
|
never lost because the runtime sends a `SchemaCacheRefreshed` after
|
||||||
|
*every* dispatch, success or failure. Covered by
|
||||||
|
`app::tests::form_b_insert_after_ddl_is_held_until_refresh_then_dispatched`
|
||||||
|
(Tier-1, deterministic event ordering) and the Tier-4 PTY regression
|
||||||
|
`e2e_pty::back_to_back_insert_after_ddl_still_succeeds` (the unpaced
|
||||||
|
inverse of flow 3).
|
||||||
|
|
||||||
## Out of scope
|
## Out of scope
|
||||||
|
|
||||||
Deliberately deferred to keep this ADR shippable as a single
|
Deliberately deferred to keep this ADR shippable as a single
|
||||||
|
|||||||
+1
-1
File diff suppressed because one or more lines are too long
+163
-1
@@ -377,6 +377,31 @@ pub struct App {
|
|||||||
/// by default; refreshed by the runtime on project load
|
/// by default; refreshed by the runtime on project load
|
||||||
/// and after successful DDL.
|
/// and after successful DDL.
|
||||||
pub schema_cache: crate::completion::SchemaCache,
|
pub schema_cache: crate::completion::SchemaCache,
|
||||||
|
/// Issue #39: `true` while a dispatched DSL command's async
|
||||||
|
/// schema-cache refresh is still in flight. The runtime applies a
|
||||||
|
/// command on the worker thread and only *afterwards* sends back a
|
||||||
|
/// `SchemaCacheRefreshed` event; until that lands, `schema_cache` is
|
||||||
|
/// stale w.r.t. the command just dispatched. Validating a follow-up
|
||||||
|
/// submission against that stale cache is the issue #39 bug (a Form-B
|
||||||
|
/// insert after `add column` sees the pre-DDL columns and is wrongly
|
||||||
|
/// rejected as "trying to write SQL?"). While this flag is set, new
|
||||||
|
/// `dispatch_dsl` submissions are *held* in `held_submissions` rather
|
||||||
|
/// than validated, then drained when the refresh arrives. Armed on
|
||||||
|
/// every `ExecuteDsl` dispatch, so at most one *DSL* command is ever in
|
||||||
|
/// flight — refreshes are then strictly one-per-dispatch and in order,
|
||||||
|
/// keeping the gate a simple, provably-correct boolean. (App-lifecycle
|
||||||
|
/// commands — `load` / `undo` / `rebuild` — also refresh the cache but
|
||||||
|
/// bypass this gate; they are modal/picker-gated and so cannot overlap a
|
||||||
|
/// rapid DSL paste, the only thing this guards.) Cleared in the
|
||||||
|
/// `SchemaCacheRefreshed` handler.
|
||||||
|
awaiting_schema_refresh: bool,
|
||||||
|
/// Issue #39: submissions deferred while `awaiting_schema_refresh` is
|
||||||
|
/// set, in submission order. Each is the canonical input line plus the
|
||||||
|
/// effective mode it was submitted under; drained through
|
||||||
|
/// `dispatch_dsl` (re-validated against the now-fresh cache) when the
|
||||||
|
/// pending refresh lands. `push_history` already ran for these at
|
||||||
|
/// `submit` time, so draining re-enters at `dispatch_dsl`, not `submit`.
|
||||||
|
held_submissions: std::collections::VecDeque<(String, EffectiveMode)>,
|
||||||
/// Whether the undo/snapshot machinery is active this session
|
/// Whether the undo/snapshot machinery is active this session
|
||||||
/// (ADR-0006 Amendment 1). `false` under the `--no-undo` CLI
|
/// (ADR-0006 Amendment 1). `false` under the `--no-undo` CLI
|
||||||
/// flag; the `undo` / `redo` commands then report undo is off
|
/// flag; the `undo` / `redo` commands then report undo is off
|
||||||
@@ -596,6 +621,10 @@ impl App {
|
|||||||
modal: None,
|
modal: None,
|
||||||
last_completion: None,
|
last_completion: None,
|
||||||
schema_cache: crate::completion::SchemaCache::default(),
|
schema_cache: crate::completion::SchemaCache::default(),
|
||||||
|
// Issue #39: no command is in flight at construction; the
|
||||||
|
// schema-refresh gate starts open with an empty hold queue.
|
||||||
|
awaiting_schema_refresh: false,
|
||||||
|
held_submissions: std::collections::VecDeque::new(),
|
||||||
// Undo is on by default; the runtime flips this off for
|
// Undo is on by default; the runtime flips this off for
|
||||||
// a `--no-undo` session (ADR-0006 Amendment 1).
|
// a `--no-undo` session (ADR-0006 Amendment 1).
|
||||||
undo_enabled: true,
|
undo_enabled: true,
|
||||||
@@ -912,7 +941,24 @@ impl App {
|
|||||||
"schema cache refreshed",
|
"schema cache refreshed",
|
||||||
);
|
);
|
||||||
self.schema_cache = cache;
|
self.schema_cache = cache;
|
||||||
Vec::new()
|
// Issue #39: the in-flight command's refresh has landed, so
|
||||||
|
// the gate opens. Drain any submissions held while it was in
|
||||||
|
// flight, re-validating each against the now-fresh cache.
|
||||||
|
// Stop as soon as a drained command dispatches (re-arming the
|
||||||
|
// gate via its own `ExecuteDsl`): the remaining held commands
|
||||||
|
// then wait for *its* refresh, preserving submission order. A
|
||||||
|
// held command that does not dispatch (parse error / pre-flight
|
||||||
|
// rejection) leaves the gate open, so the loop continues to the
|
||||||
|
// next held submission.
|
||||||
|
self.awaiting_schema_refresh = false;
|
||||||
|
let mut actions = Vec::new();
|
||||||
|
while !self.awaiting_schema_refresh {
|
||||||
|
let Some((input, submission_mode)) = self.held_submissions.pop_front() else {
|
||||||
|
break;
|
||||||
|
};
|
||||||
|
actions.extend(self.dispatch_dsl(&input, submission_mode));
|
||||||
|
}
|
||||||
|
actions
|
||||||
}
|
}
|
||||||
AppEvent::RelationshipsRefreshed(relationships) => {
|
AppEvent::RelationshipsRefreshed(relationships) => {
|
||||||
trace!(count = relationships.len(), "relationships refreshed");
|
trace!(count = relationships.len(), "relationships refreshed");
|
||||||
@@ -1981,6 +2027,20 @@ impl App {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn dispatch_dsl(&mut self, input: &str, submission_mode: EffectiveMode) -> Vec<Action> {
|
fn dispatch_dsl(&mut self, input: &str, submission_mode: EffectiveMode) -> Vec<Action> {
|
||||||
|
// Issue #39: if a previously-dispatched command's schema-cache
|
||||||
|
// refresh is still in flight, `schema_cache` is stale w.r.t. that
|
||||||
|
// command. Validating this submission now would race it (the bug:
|
||||||
|
// a Form-B insert after `add column` rejected against the pre-DDL
|
||||||
|
// schema). Hold it in submission order; the `SchemaCacheRefreshed`
|
||||||
|
// handler drains the queue once the fresh schema lands. App-level
|
||||||
|
// commands (`quit`, `help`, `load`, …) route through
|
||||||
|
// `dispatch_app_command` *before* here, so they are never held.
|
||||||
|
if self.awaiting_schema_refresh {
|
||||||
|
debug!(input, "holding submission until schema cache refresh lands");
|
||||||
|
self.held_submissions
|
||||||
|
.push_back((input.to_string(), submission_mode));
|
||||||
|
return Vec::new();
|
||||||
|
}
|
||||||
// The two-way mode the walker + the `[mode]` render tag read; the
|
// The two-way mode the walker + the `[mode]` render tag read; the
|
||||||
// three-way `submission_mode` (ADR-0037) rides on `ExecuteDsl` for
|
// three-way `submission_mode` (ADR-0037) rides on `ExecuteDsl` for
|
||||||
// the runtime's echo gate (ADR-0038).
|
// the runtime's echo gate (ADR-0038).
|
||||||
@@ -2069,6 +2129,14 @@ impl App {
|
|||||||
}];
|
}];
|
||||||
}
|
}
|
||||||
self.push_output(OutputLine::echo(input, mode));
|
self.push_output(OutputLine::echo(input, mode));
|
||||||
|
// Issue #39: a command is now in flight; its schema-cache
|
||||||
|
// refresh will land asynchronously. Arm the gate so any
|
||||||
|
// follow-up submission waits for the fresh schema rather
|
||||||
|
// than racing the stale cache. Armed on every dispatch (not
|
||||||
|
// just DDL) so only one command is ever in flight, which
|
||||||
|
// keeps the gate a simple boolean — refreshes are then
|
||||||
|
// strictly one-per-dispatch and in order.
|
||||||
|
self.awaiting_schema_refresh = true;
|
||||||
vec![Action::ExecuteDsl {
|
vec![Action::ExecuteDsl {
|
||||||
command: cmd,
|
command: cmd,
|
||||||
source: input.to_string(),
|
source: input.to_string(),
|
||||||
@@ -4614,6 +4682,100 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn form_b_insert_after_ddl_is_held_until_refresh_then_dispatched() {
|
||||||
|
// Issue #39: a simple-mode Form-B insert (`insert into T values
|
||||||
|
// (…)`, no column list) submitted faster than the post-DDL
|
||||||
|
// schema-cache refresh must NOT be validated against the *stale*
|
||||||
|
// cache — doing so wrongly rejects it as "trying to write SQL?".
|
||||||
|
//
|
||||||
|
// This reproduces, deterministically, the event ordering the async
|
||||||
|
// runtime produces under fast input: a schema-mutating command is
|
||||||
|
// dispatched (arming the gate while its cache refresh is in flight),
|
||||||
|
// a follow-up insert is submitted before the refresh lands, and only
|
||||||
|
// then does the `SchemaCacheRefreshed` event arrive. Pre-fix the
|
||||||
|
// insert is validated against the pre-DDL schema and rejected; with
|
||||||
|
// the gate it is held and dispatched against the fresh schema.
|
||||||
|
use crate::completion::{SchemaCache, TableColumn};
|
||||||
|
use crate::dsl::types::Type;
|
||||||
|
|
||||||
|
// Pre-DDL schema: `Customers` has only the auto `id` (serial). This
|
||||||
|
// is the stale cache a racing Form-B insert would otherwise see —
|
||||||
|
// zero user-fillable columns, so `values ('Alice')` can't match.
|
||||||
|
let mut app = App::new();
|
||||||
|
let mut stale = SchemaCache::default();
|
||||||
|
stale.tables.push("Customers".to_string());
|
||||||
|
stale.columns.push("id".to_string());
|
||||||
|
stale.table_columns.insert(
|
||||||
|
"Customers".to_string(),
|
||||||
|
vec![TableColumn {
|
||||||
|
name: "id".to_string(),
|
||||||
|
user_type: Type::Serial,
|
||||||
|
not_null: true,
|
||||||
|
has_default: false,
|
||||||
|
}],
|
||||||
|
);
|
||||||
|
app.schema_cache = stale;
|
||||||
|
|
||||||
|
// 1. Submit the DDL. It dispatches and arms the gate: a schema
|
||||||
|
// refresh is now (conceptually) in flight.
|
||||||
|
type_str(&mut app, "add column to Customers: Name (text)");
|
||||||
|
let ddl_actions = submit(&mut app);
|
||||||
|
assert!(
|
||||||
|
matches!(ddl_actions.as_slice(), [Action::ExecuteDsl { .. }]),
|
||||||
|
"the DDL should dispatch; got {ddl_actions:?}",
|
||||||
|
);
|
||||||
|
|
||||||
|
// 2. Submit the Form-B insert *before* the refresh lands. It must be
|
||||||
|
// held — not dispatched, and crucially not rejected against the
|
||||||
|
// stale cache (no error note such as "trying to write SQL?").
|
||||||
|
type_str(&mut app, "insert into Customers values ('Alice')");
|
||||||
|
let held_actions = submit(&mut app);
|
||||||
|
assert!(
|
||||||
|
held_actions.is_empty(),
|
||||||
|
"the insert must be held while the refresh is in flight, \
|
||||||
|
not dispatched or rejected; got {held_actions:?}",
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!app.output.iter().any(|l| l.kind == OutputKind::Error),
|
||||||
|
"a held insert must produce no error note (e.g. the \
|
||||||
|
'trying to write SQL?' pointer); errors:\n{}",
|
||||||
|
error_lines(&app),
|
||||||
|
);
|
||||||
|
|
||||||
|
// 3. The DDL's schema refresh lands carrying the fresh schema
|
||||||
|
// (`Customers` now has `id` + `Name`). The held insert drains and
|
||||||
|
// dispatches, validated against the up-to-date cache.
|
||||||
|
let mut fresh = SchemaCache::default();
|
||||||
|
fresh.tables.push("Customers".to_string());
|
||||||
|
fresh.columns.push("id".to_string());
|
||||||
|
fresh.columns.push("Name".to_string());
|
||||||
|
fresh.table_columns.insert(
|
||||||
|
"Customers".to_string(),
|
||||||
|
vec![
|
||||||
|
TableColumn {
|
||||||
|
name: "id".to_string(),
|
||||||
|
user_type: Type::Serial,
|
||||||
|
not_null: true,
|
||||||
|
has_default: false,
|
||||||
|
},
|
||||||
|
TableColumn::new("Name", Type::Text),
|
||||||
|
],
|
||||||
|
);
|
||||||
|
let drained = app.update(AppEvent::SchemaCacheRefreshed(fresh));
|
||||||
|
assert!(
|
||||||
|
matches!(
|
||||||
|
drained.as_slice(),
|
||||||
|
[Action::ExecuteDsl {
|
||||||
|
command: Command::Insert { .. },
|
||||||
|
..
|
||||||
|
}]
|
||||||
|
),
|
||||||
|
"the held insert must dispatch once the fresh schema lands; \
|
||||||
|
got {drained:?}",
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn simple_mode_submit_of_pure_dsl_error_has_no_advanced_pointer() {
|
fn simple_mode_submit_of_pure_dsl_error_has_no_advanced_pointer() {
|
||||||
// A DSL error that is *not* valid SQL either (unknown command)
|
// A DSL error that is *not* valid SQL either (unknown command)
|
||||||
|
|||||||
@@ -377,6 +377,33 @@ fn undo_after_drop_table_restores_it() {
|
|||||||
app.quit();
|
app.quit();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Flow 5 — issue #39 regression: commands submitted **back-to-back**, with
|
||||||
|
/// no wait between them, must still execute correctly. This is the inverse of
|
||||||
|
/// flow 3 (which paces each command on purpose). Pre-fix, a Form-B insert sent
|
||||||
|
/// immediately after `add column` was validated against the *stale* schema
|
||||||
|
/// cache — the worker hadn't yet refreshed it — and wrongly rejected as
|
||||||
|
/// "trying to write SQL?", so the row never landed. The schema-refresh gate
|
||||||
|
/// (`App::awaiting_schema_refresh`) now holds each submission until the prior
|
||||||
|
/// command's refresh arrives, making the outcome independent of input speed.
|
||||||
|
#[test]
|
||||||
|
fn back_to_back_insert_after_ddl_still_succeeds() {
|
||||||
|
let mut app = PtyApp::launch(&[]);
|
||||||
|
app.wait_for_no_tables(); // fresh project
|
||||||
|
|
||||||
|
// Fire all three with no readiness wait in between — the faster-than-human
|
||||||
|
// input that triggered issue #39 (paste / script / unpaced driver).
|
||||||
|
app.submit("create table Customers with pk id(serial)");
|
||||||
|
app.submit("add column to Customers: Name (text)");
|
||||||
|
app.submit("insert into Customers values ('Alice')");
|
||||||
|
|
||||||
|
// The insert's OWN success echo (value + ✓) — proof the row reached the
|
||||||
|
// database, not the "trying to write SQL?" rejection. If the gate
|
||||||
|
// regressed, the insert would misparse against the stale schema and this
|
||||||
|
// would time out.
|
||||||
|
app.wait_for("('Alice') ✓");
|
||||||
|
app.quit();
|
||||||
|
}
|
||||||
|
|
||||||
// ===================== NFR perf (measured, generous) ===================
|
// ===================== NFR perf (measured, generous) ===================
|
||||||
//
|
//
|
||||||
// These run against the DEBUG binary, so the bounds are loose
|
// These run against the DEBUG binary, so the bounds are loose
|
||||||
|
|||||||
@@ -170,6 +170,12 @@ fn colon_escape_in_simple_mode_is_one_shot() {
|
|||||||
echoed.text,
|
echoed.text,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Issue #39: dispatching `:select 1` arms the schema-refresh gate (a
|
||||||
|
// cache refresh is conceptually in flight). In the real runtime that
|
||||||
|
// refresh lands before the user types the next line; model it here so
|
||||||
|
// the follow-up submission is processed rather than held.
|
||||||
|
app.update(AppEvent::SchemaCacheRefreshed(app.schema_cache.clone()));
|
||||||
|
|
||||||
// Subsequent submission (unrecognised in simple mode) parse-errors,
|
// Subsequent submission (unrecognised in simple mode) parse-errors,
|
||||||
// not echoes — confirming the mode reverted.
|
// not echoes — confirming the mode reverted.
|
||||||
type_str(&mut app, "list things");
|
type_str(&mut app, "list things");
|
||||||
|
|||||||
Reference in New Issue
Block a user