Skip to content

fix: defects found by the pre-production audit, including two regressions - #4

Merged
JustGodWork merged 1 commit into
developfrom
fix/audit-followup
Jul 22, 2026
Merged

fix: defects found by the pre-production audit, including two regressions#4
JustGodWork merged 1 commit into
developfrom
fix/audit-followup

Conversation

@JustGodWork

Copy link
Copy Markdown
Owner

Follow-up to a full re-audit of the three stacked PRs. Six fixes, two of which are regressions introduced by the earlier batch.

Regressions from the previous fixes

The pcall around driver calls invoked the callback twice. A driver that calls back synchronously runs the callback inside that pcall, so an error raised by the continuation came back as ok == false and was reported as a second call. With error(nil) the second call took the success branch and re-entered the state machine of sync() and migrate(), re-emitting DDL and re-inserting the norm_migrations row. It also produced an ERROR statement failed log and a _fail_queue for a statement that had succeeded.

utils.protected now guarantees the single invocation adapter.lua documents, and logs a continuation error rather than turning it into a query failure.

The soft-delete scope added to the upsert read-back was wrong. ON DUPLICATE KEY UPDATE and ON CONFLICT are not scoped, so the engine updates a trashed row happily. Filtering the read-back with deleted_at IS NULL then resolved nil for a write that had actually happened, and a caller retrying on nil would loop forever. Removed.

Incomplete fixes from the previous batch

sync() only purged the queue from the error callback. An adapter that raises instead of calling back, which is what a closed connection or a not-yet-started driver does, bypassed it and stranded every queued operation with its promise pending. That is the exact failure the queue fix was meant to remove.

Marking index statements optional was too broad. Since MySQL never supports IF NOT EXISTS on an index, every CREATE INDEX failure was swallowed there, including an unknown column on a UNIQUE index. A missing unique index silently removes the race guarantee that upsert and find_or_create rely on. Only a duplicate-index error is skipped now, everything else fails sync() as before.

Two nil holes remained in the family the previous batch addressed. having(expr, nil) in its two-argument form emitted a placeholder with no parameter, so the next HAVING value bound to the wrong slot. where_in(col, nil) compiled to col IS NULL, exactly like where_not_in(col, nil), despite meaning the opposite: an authorisation filter built from a nil list stopped filtering silently.

Eager loading still shared one collection table between parents holding the same source key, which is legal with a non-unique localKey.

Not addressed here

The audit also surfaced pre-existing defects outside the scope of these fixes, listed for the record: with_count is lost when combined with include, paginate ignores include entirely, after_save and after_update do not fire when save() has nothing to write, attach/detach ignore otherLocalKey, a duck-typed adapter without default_provider crashes at construction, soft delete mutates the record before the write is confirmed, update({}) builds an empty SET, and onDelete/onUpdate are concatenated into the DDL without validation.

Suite: 357 passing, 341 before this branch.

Two of them were introduced by the previous batch of fixes.

Regression, adapters: the pcall added around the driver call could invoke
the callback twice. A driver that calls back synchronously runs the
callback inside that pcall, so an error raised by the continuation came
back as ok == false and was reported as a second call. With error(nil)
the second call even took the success branch and re-entered the state
machine of sync() and migrate(), re-emitting DDL and re-inserting the
norm_migrations row. utils.protected now guarantees the single
invocation the adapter contract requires, and logs a continuation error
instead of turning it into a query failure.

Regression, upsert: the soft-delete scope added to the read-back was
wrong. ON DUPLICATE KEY / ON CONFLICT is not scoped, so the engine
updates a trashed row happily, and filtering the read-back resolved nil
for a write that had happened. A caller retrying on nil would loop.

sync() only called _fail_queue from the error callback, so an adapter
raising instead of calling back still stranded every queued operation,
which is the exact failure the queue fix targeted.

Marking index statements optional was too broad: on MySQL every
CREATE INDEX failure was swallowed, including an unknown column on a
UNIQUE index, which silently drops the guarantee upsert and
find_or_create depend on. Only a duplicate-index error is skipped now.

Two nil holes remained in the family the previous batch addressed:
having(expr, nil) in its two-argument form emitted a placeholder with no
parameter, and where_in(col, nil) compiled to the same `col IS NULL` as
where_not_in(col, nil) despite meaning the opposite.

Eager loading still shared one collection table between parents holding
the same source key, which is legal with a non-unique localKey.

Adds 16 regression tests. Suite: 357 passing.
@JustGodWork
JustGodWork changed the base branch from pr/adapters-promises-and-ci to develop July 22, 2026 01:06
@JustGodWork
JustGodWork merged commit b6e38f0 into develop Jul 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant