Work through the remaining correctness findings from the static-analysis sweep - #729
Merged
Conversation
…t state Six failure paths treated a recoverable error as a terminal one, and two more let concurrent work overwrite each other. `refreshAccessToken` mapped any 400 or 401 to a revocation without reading the body, so a rotated Maple client secret — `invalid_client`, which RFC 6749 sends as a 400 — would have revoked every tenant of that provider at once. Only `invalid_grant` means the grant is gone; everything else is now a non-mutating upstream failure. The rotated refresh token is also retried on a persistence blip rather than discarded, which previously lost the only valid token. Capped upstream listings were reconciled as complete: the Cloudflare and PlanetScale pollers stopped at their page ceiling and then disabled, soft-deleted or filtered by what they had seen. The caps stay — they are a deliberate bound — but truncation is now observable, and every destructive pass is skipped when the listing it would act on was incomplete. The deploy-request watermark likewise holds rather than claiming unscanned requests as done. A Slack workspace lookup failing on a database error was reported as an authentication failure, which is never retried and counts toward destination auto-disable; it is a retryable delivery failure now. Slack revocation read a snapshot, revoked its API key, then wrote by row id, so a concurrent reinstall could be marked revoked with its fresh key left active and orphaned. The write is a compare-and-set and the credential revocation follows the transition rather than preceding it. Schema application could wedge permanently: the run row was committed `queued` before the binding was validated, and a workflow that never started left every later request answering `already_running`. The claim is now one conditional upsert that a stale run releases after 30 minutes, and the workflow marks itself failed on the configuration step it previously died before reaching. That status response also carries the migrations it skipped, which it silently swallowed — a non-gating migration could leave error processing down behind a green UI. The APNs provider token minted once per concurrent sender on a cold cache, and a model response containing two completion calls executed both.
…edges Migration 0009 inserted additively into `IF NOT EXISTS` targets and dropped its views only at the end, so any later failure meant the next apply replayed both backfills over rows that were already there — doubling every `sum` across a year of retention. It is reordered to the cutover shape 0015 already uses: drop views, truncate, backfill, reattach. A server that recorded v9 never re-runs it, and a fresh install truncates empty tables, so the change only affects the partially-failed apply it exists to fix. An invariant test now requires every backfill in every migration to be preceded by a truncate, a fresh-table rebuild, or a scoped delete of the rows it writes. The other migrations already complied; the point is that the next one has to. The service-map rollup was double-counting for BYO-ClickHouse orgs on every hourly tick, without bound. Its idempotency probes resolved as ordinary reads, which for a BYO org means that org's own cluster — where the rollup has never written anything, because ingest is managed-only by design. Every hour looked unrolled, so the same additive edge rows were re-rolled and re-ingested. Both seal probes are pinned to the ingest backend, matching how `alert_checks` already does it. Managed orgs are unaffected. The drop-and-recreate gaps in 0015 through 0020 are left alone: each is documented, non-gating on purpose, and the ingest gateway diverts writes for the whole window of a gating apply. What was not deliberate was that a failure there was invisible, which the apply-status change in the previous commit fixes.
…thin evidence `applyTransition` wrote the issue update, incident resolution, state clear and timeline event as four separate executions, and its retry returns early when the issue already sits in the target state — so a partial commit could never be repaired. All four now commit together, as `AlertRulesService` already does. Severity escalation had the same shape and the same consequence: a committed severity change whose page was lost, permanently, because the retry short-circuited on "nothing changed" before reaching the outbox. Fix verification could auto-close an issue that was still failing. The occurrence split discarded rows with an empty `serviceVersion` even though the comment above it said their presence makes the verdict inconclusive, so a version-less service still throwing looked exactly like one that had gone quiet. The row limit compounded it — the query takes the hundred largest counts, but the decision is about membership, not the head. Unattributed and truncated evidence now re-arm the window instead of certifying it. Verdict writes matched on id alone while the error tick could terminally settle the same row, so a stale snapshot could overwrite a refutation with `verified` and close the issue. Every status write is a compare-and-set on the snapshot it was decided from, and a lost race is logged rather than applied. The verdict and its event commit together, so a failure leaves the row due again next tick instead of stranding the issue in `verifying`. The every-minute worker ran its four ticks at `concurrency: 2` and relied on array order to keep fix verification behind the error tick, which bounded concurrency does not provide — the two ran together as soon as a slot freed. The dependency is explicit now and the unrelated ticks stay parallel. Automatic investigation fan-outs never recorded their workflow instance, so a restart could not terminate the run it was replacing. Also carries the merged-link guard in `onPullRequestEvent`: a pre-merge webhook delivered after `closed(merged)` regressed the link and nulled the merge metadata the verification window was opened from. A merge is irreversible, so a non-merged event arriving at a merged link is stale by definition.
…d data The sync queue runs two invocations concurrently, retries immediately, and can re-enqueue a rate-limited continuation up to a day later, so a job holding a decoded repository or installation across a provider round-trip is routinely stale by the time it writes. Two installation-sync jobs for different installations of one org each purged every sibling with no ordering between them, so both could delete the other and leave the org fully disconnected. "Supersedes" is now a strict order on creation time — only strictly-older siblings are purged — which makes mutual purge impossible by construction and needs no lock. A stale worker could also write children under a parent that had just been purged, and a purged private commit stayed readable through the `(org_id, sha)` lookup because that read never joined its repository. There is deliberately no foreign key here, so the link is enforced where the writes are: child upserts take a share lock on the parent row inside their transaction and no-op when it is gone, purges delete the parent before its children, and the installation purge takes its repository snapshot inside the transaction rather than before it. Either the upsert holds the lock and the purge's sweep removes what it just wrote, or the purge wins and the upsert writes nothing. The commit lookups join their repository, so even a hand-made orphan is unreadable. A retarget wipes the old branch's commits, but a queued job for the old branch would refill them and mark the repo ready — backfills only upsert, so the mix was permanent. Jobs compare their branch against the tracked branch at start and again after the provider fetch. An exhausted job for a branch that is no longer tracked also stopped flagging the new branch's healthy backfill as errored.
A delivery that timed out kept running on the wire. `Effect.timeout` does interrupt the effect it wraps, but the request was built with a `tryPromise` callback that never took the abort signal, and without it interruption cannot reach the fetch — so "timed out, will retry" raced a first delivery that was still in flight, and paged twice. The signal is threaded through both the direct call and the guarded one, which already carries it across redirect hops. Duplicate open incidents had no database idempotency at all: the unique index meant to prevent them was keyed on the incident's own freshly generated id, so it constrained nothing. Both incident tables get a partial unique index over the open row's identity, and the insert yields to a conflict instead of opening a second one. Migration 0054 resolves the duplicates that already exist — it must, because the index cannot be created while they do. A destination lookup failing on a database error was substituted with an empty result, so every destination reported missing, the escalation finalized as "no enabled destinations", and the page was dropped for good. A lookup failure is now a failure; `missing` means the query succeeded and found nothing. Delivery leases were computed once at the head of the batch and applied to rows processed one after another, so a row claimed after thirty seconds of slow sending was born with an expired lease and could be reclaimed mid-delivery. Each row reads the clock when it claims, which costs nothing and makes the lease strictly outlive the delivery timeout. Permanent Slack failures — revoked tokens, archived channels, invalid payloads — were retried five times and never counted toward the failure streak that disables a destination. Unknown codes stay retryable on purpose: calling a transient failure terminal costs a destination its enablement, while the reverse only wastes retries. A recovered destination could also be disabled by a decision made before it recovered, since the streak increment and the disable were two statements; they are one now. Log anomaly coverage no longer caps on arbitrary query order, and the liveness probe scopes to the rule's environments — it already had the predicate and was not using it, so staging traffic could certify a production recovery.
Archive GC could delete every generation in a range. The active pointer was read as an opaque string with no check that it selects a verified generation, so a well-formed but stale pointer made all of them superseded and put the whole range in the delete set — and the revalidation before deleting compares the same stale value, so nothing downstream caught it. Planning now parses the pointer like every other read path and requires it to select exactly one verified generation, excluding the range as uncertain otherwise. The checkpoint client gave BACKUP thirty seconds. The server runs it through a synchronous call that cannot observe an aborted request, so the timeout unwound the caller and released maintenance ownership while the backup was still writing — and the retry would then rename the directory out from under it. A large store legitimately takes minutes; a dead server surfaces as a connection error anyway. Durable writes hardened the parent directory of whatever they wrote, and those paths are siblings of the caller's `--data-dir`, so pointing the CLI at `/var/lib/maple` re-permissioned `/var/lib`. Missing parents are still created private; existing ones are left as the operator set them. The legacy replay decoded raw 64-bit columns as JSON numbers, silently rounding durations and histogram counts above 2^53 — after which the inventory hash fails the migration outright. Both replay queries now ask for quoted 64-bit output, and the fetch bounds itself by observed bytes instead of materializing thousands of rows before the byte limit is applied. Two starts could both open the same store natively, because the pid file was written only after the listener came up; it is claimed exclusively first. An abort released its checkpoint pin before recording that it had aborted, which left the operation permanently unrecoverable — the terminal phase is written first and the cleanup is resumable. Migration staging no longer clones the backups tree, which arrived with a fingerprint the new schema rejects, making every checkpoint unusable at exactly the moment the CLI advises taking one. Self-update no longer shares a scratch directory between concurrent updaters and restores the matched pair if either rename fails.
An org switch could execute a request against the wrong tenant. When the identity generation moved mid-resolve, the header builder re-resolved under the new identity and sent the request — built for one org — with another org's bearer, and the API scopes tenancy by that bearer. The existing tests asserted this, because every case they covered starts from a null org at boot, where re-resolving is right. That case keeps its retry; a request that started under a real org now fails before dispatch instead of crossing tenants. Chart downsampling could hide the breach the chart exists to show. Past 720 points it kept every nth sample, so a one-window spike on a skipped index simply vanished — and alert previews render up to 1,500 windows. Buckets now keep their minimum and maximum, so a breaching sample is always at an extremum of its bucket and always survives, in both comparator directions. Two rule templates were wrong. The throughput-drop preset inherited the form's default minimum sample count, and the evaluator skips low-sample windows before comparing — so for a rule whose signal is the sample count, the severe drops, including an outage, were never evaluated. The high-error-rate preset left grouping empty and evaluated one org-wide ratio; the same template on the tool side has defaulted to grouping by service for exactly this reason. Alert staleness was scaled by the query window, so a rule with a 24-hour window stayed "healthy" for 72 hours after the scheduler stopped. The scheduler claims every enabled rule every minute regardless of window, so the threshold is a function of the heartbeat, not the window. A rule that has been scheduled but has never completed an evaluation now warns rather than reporting all stages passing. Dashboard documents from a newer schema version were being rewritten and restamped by the upgrader that was supposed to leave them alone, and a legacy sparkline transform made whole documents undecodable, locking the dashboard out of the writable path. The browser SDK released the global tracer registration on shutdown, so a second init after a shutdown exported nothing; it now releases only what it registered and leaves a host application's provider alone.
… nullability `reset-preview-branch` dropped everything at `DATABASE_URL` whenever `CI` was set to anything — including the string "false" — and never checked that the target was a preview branch. The environment bypass is gone: the caller has to name a `pr-<n>` branch, or a human has to confirm explicitly. It is reachable only from the dormant preview workflow today, which is the reason it was worth fixing rather than the reason to leave it. `min` and `max` declared a non-nullable result while their codec stayed nullable, and the histogram columns they read genuinely are nullable — so an all-NULL bucket failed the metrics request against a contract that promised a number. The signatures tell the truth now, which surfaced the one query relying on the wrong inference; it falls back to zero at the SQL level, matching the average on the same select. `ifNull` is added to the builder because the existing coalesce cannot express the narrowing its own runtime rule performs. The dashboard datasource backfill kept its recovery dump in memory and wrote it once at exit, so the preimages it promised to flush before each batch did not exist when a crash needed them. It is an fsynced journal now, and restore verifies it is overwriting the payload the backfill actually wrote — it matched on version alone, which after a concurrency miss is the user's own edit — and leaves the version counter moving forward rather than rolling it back. Standalone privilege setup keyed default privileges to the role it switched to, while migrations in a separate process create objects as whatever role logged in; on a stage where those differ, the defaults never applied. Both candidate roles are covered. A scraper configured with zero concurrency built a semaphore that never grants a permit, suspending every target while health checks stayed green; the numeric settings are bounded integers that fail at startup. A stalled ingest request could also hold a permit indefinitely, so delivery carries the same timeout the rest of the client already had.
Review caught that several fixes in this stack decided things with ad-hoc
structural checks where a primitive belongs — the worst of them reconstructing
which shape a transaction returned by sniffing for a key:
if ("missingDestinationId" in writeResult && writeResult.missingDestinationId !== undefined)
`writeRuleRow` is the reference for the shape that replaces it. A drizzle
transaction body is Promise-land and cannot fail in the Effect channel, so it
returns a `_tag`-discriminated outcome and the caller raises the failures through
`Match.value(...)` with `Match.exhaustive` — which makes a fourth outcome a
compile error instead of a branch that quietly falls through. `CloudflareApiImpl`
gets the same treatment against an SDK union that carries no tag, matching each
variant on its distinguishing field, so a new origin kind can no longer arrive as
a silently null host and port.
Elsewhere the fix is to stop discarding an Option that was already in hand:
`oauthErrorCodeOf` was Option-based internally and collapsed to `string | null`
at its edge, the APNs token cache held `entry | null` where absence and expiry
are the same thing, and the update sweep unwrapped an Option with
`getOrUndefined` only to compare it against undefined. The dispatcher's null was
manufactured by a `catchTag` and is now `Effect.succeedNone`, which keeps a
failed lookup from reading as a missing row.
The remaining conversions are `Arr.head`/`Option` in place of index-and-compare,
`Arr.headNonEmpty`/`lastNonEmpty` behind a proven-nonempty guard in place of the
`!` assertions in chart downsampling, and a decoded `Schema.Struct` in place of
`JSON.parse(...) as JournalLine` in the backfill journal — where a required
`upgraded_json` now makes a pre-verification dump fail decode rather than
restore blind.
Not every flagged line moved. `Predicate.isNull`/`isNotUndefined` is the right
primitive where a value is tested and never unwrapped, and wrapping those in
Option would be ceremony. The `packages/db` scripts genuinely run outside the
Effect runtime and keep their driver-seam try/catch and the preview-reset guard
exactly as they are. The CLI's errno casts sit inside `Effect.try` at the node
boundary and match the convention of twenty pre-existing sites, and the widget
migration's structural probe is the boundary conversion for opaque stored JSON.
No behaviour changes: every regression test from this stack still passes.
The native calibrate probe failed once in CI with "every eligible candidate failed held-out validation ... or the data was insufficient for a complete six-signal held-out split", and passed on a rerun with no code change. The checkpoint was not at fault: its own manifest records 30 rows validated for all six signals. The probe was simply seeded to the exact minimum. Calibration trains on rows [0, sampleRows) and validates on [sampleRows, sampleRows * (1 + HELD_OUT_SAMPLE_MULTIPLIER)) — with --sample-rows 10 and a multiplier of 2 that is 30 rows per signal, while the comment above the seeder asserted 20 were enough and seeded exactly 30. At zero margin the whole probe turns on the split placing every single row, and the failure surfaces as all four candidates failing at once rather than as anything pointing at the seed. Seeds 45 per signal instead, and states the derivation so the requirement and the seed cannot drift apart again.
`BoundedListing` is consumed only inside `CloudflareApiImpl`, so re-exporting it from the barrel added a name nothing imports. knip reported it; the other six findings in that report predate this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🍁 Maple PR previewWarning Preview cleanup could not be confirmed. The Alchemy teardown outcome was Final commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes out the correctness tier of the analysis sweep: all 95 findings that were left
untriaged after #721–#725. 75 were fixed; 20 were verified against the tree and rejected
as deliberate trades or scoped out as their own projects. None were noise — every finding
described something real, though several described it for the wrong reason.
Each fix carries a regression test demonstrated to fail against
mainbefore it passes.In one case the existing suite asserted the buggy behaviour as correct, and that assertion
is now the test for the fix.
Read as 8 commits, not one diff
The commits are self-contained by subsystem and are the intended review unit:
ce4aac9b363dbc29fc6ba7483283a2b651774c5508de3d08b929516151db46edIf you would rather review this as a dependent stack, the branches exist locally as
fix/07…fix/14and I can split it — say the word and I will push the other seven.The ones worth reading first
org. The rollup's idempotency probes resolved as ordinary reads, which for a BYO org
means that org's own cluster — where the rollup has never written, because ingest is
managed-only by design. Every tick judged the whole window unrolled and re-ingested it.
This is live, not hypothetical.
Effect.timeoutdoes interrupt, but therequest was built with a callback that never accepted the abort signal, so interruption
could not reach the fetch — "timed out, will retry" raced a first delivery that was still
in flight.
builder re-resolved mid-flight and dispatched a request built for one org under another
org's bearer.
version-less occurrences that its own comment said should make the verdict inconclusive.
every backfill in every migration to be preceded by a truncate, a rebuild, or a scoped
delete, so the class cannot come back.
Migration 0054 needs its pre-sweep read before applying
Adds partial unique indexes over open
alert_incidentsandanomaly_incidents. Live dataalready holds the duplicate open rows that are the bug, and
CREATE UNIQUE INDEXfailswhile they exist, so the migration resolves all but the freshest per key first. Production
migrations are applied by hand — please read that sweep rather than running it blind.
What this deliberately does not do
Named rather than half-done, each its own piece of work: optimistic concurrency for
concurrent alert-rule edits; bounding the
alert_incidentssync (must be a server-pinnedshape, which runs straight into the churn problem we reverted before); exactly-once
warehouse backfill chunks; tenant-following derived writes so BYO orgs can read the hourly
service-map interior; a transactional outbox for the commit-then-enqueue paths; and pidfile
process identity.
No foreign keys were added — the two findings that wanted referential integrity are
enforced in application code instead, matching house style.
Verification
13 packages typecheck; every touched suite passes;
bun run lintandoxfmt --checkclean.apps/web guided-setup.test.tsxfails locally for anyone whose.env.localselects Clerkmode — it is env-derived, pre-existing, and passes in CI.
Per-commit standalone typecheck has not been run, because the worktree is shared with
another active session and checking out intermediate commits would disrupt it.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.