You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A blocked record that says it retries on a timer waits for a person instead. admission.NextBlockedRetry names the reasons that should come round again, computes when each is next due, and nothing calls it. The only path that re-offers a blocked record is basecamp connect redispatch <id>, one id at a time — so a read that failed during an outage, a throttle, and a connect.json that could not be read all sit until somebody notices and redispatches each one. Repairing the file, or the outage ending, clears nothing by itself.
Several comments across the connector say otherwise — "retained and retried", "recovered on the blocked schedule". They were left standing rather than corrected, because building this is what makes them true. This builds it, and the ones it does not make true are corrected here.
The intake sweep that already re-offers stranded events and open losses now also asks the ledger which blocked records are due and offers those. The schedule stays where it was — NextBlockedRetry is still the one place that says when a record is next owed an attempt; the ledger's query only narrows to the rows it could possibly say yes about, and the sweep only moves them.
Three things the earlier attempt was taken out over, each addressed here rather than assumed away.
Scope. The query takes the run's --project buckets and will not return a record outside them. out_of_scope is a terminal discard and the projects a run leaves out are another run's to dispatch, so a retry that crossed the scope would cause the permanent loss it exists to prevent. Every other terminal verdict a re-offered record can reach — stale, not_addressed, untrusted_performer, agent_authored — is the verdict a fresh event would get, and is correct.
The claim.Queue.Offer does not deduplicate, so a due row is claimed before it is offered and the claim is given back if the offer fails. Without one the same rows requeue on every tick while the rows behind them in the window starve, and a second copy can decide a record the moment the first re-blocked it, bypassing the interval. The claim is a map from record to the revision it was last offered at: a verdict bumps the revision, so a record's own next decision retires its claim. In memory is enough — AcquireInstanceLock is a flock the kernel drops on process death, so exactly one connector ever sweeps a ledger, and a crash costs at most one extra decision, which admission's revision guard already makes harmless.
The join. The removed version's end-to-end test called the query and the queue itself, so it stayed green with the periodic hook deleted. There are two tests here: one drives the sweep directly, one drives the real ticker with nothing else offering anything. Deleting the hook was checked against both — the ticker test fails, the sweep test stays green.
Which reasons this turns on, which it does not
Wiring this enables automatic re-decision for every reason the schedule names. That is a feature, not a defect fix, so it is worth saying plainly what it decides now:
read_failed — retried, 24h. The case the schedule exists for. It covers 401/403/404 too, which heal when access is granted or a token is refreshed; the window is what stops a deleted recording being re-read forever.
read_unresolved — retried, 24h. A chat line under no visible Campfire; visibility changes when a project is granted.
delta_unverified — retried, 24h. The assignment event fell outside the events read's bound, and a later read can find it.
trust_unverified — retried, 24h. Time is the entire remedy: the only listing at hand predates the event and a fresh one was read too recently to read again.
throttled — retried, 24h, never before the server's own deadline.
config_unreadable — retried, with no window. This is the one change to the schedule itself. A day is the right bound for a failing server: after one, it is not coming back on its own and a person should see the record. A broken connect.json is a local file, and an operator away for a week is ordinary — giving up there strands the work silently.
no_route, bucket_mismatch, unroutable — not retried, unchanged. The first waits on the operator serving the project, which is a decision rather than a delay; the other two are facts about the pointer that no later read changes.
Nothing wanted excluding, so the reason set lives in the query rather than in the scheduler: one list, shared by the function that dates a record and the query that finds it, with a test that they cannot drift apart.
A due row the schedule has finished with stays blocked forever, so the query pages past those rather than cutting the scan off with a LIMIT — otherwise they would hold a place in every sweep's window for good.
admission.NextBlockedRetry names the blocked reasons that should come round
again, computes when each is next due, and nothing called it. The only path
that re-offered a blocked record was `basecamp connect redispatch <id>`, one
id at a time, so a read that failed during an outage, a throttle, and a
connect.json that could not be read all sat until a person noticed each one.
The intake sweep that already re-offers stranded events and open losses now
also asks the ledger which blocked records are due and offers those. The
schedule stays where it was: NextBlockedRetry is still the one place that says
when a record is owed an attempt, the ledger's query narrows to the rows it
could say yes about, and the sweep only moves them.
Three things the attempt removed from #765 was taken out over:
- Scope. The query takes the run's --project buckets and will not return a
record outside them. out_of_scope is a terminal discard and the projects a
run leaves out are another run's to dispatch, so a retry that crossed the
scope would cause the permanent loss it exists to prevent.
- The claim. Queue.Offer does not deduplicate, so a due row is claimed
before it is offered and the claim is given back if the offer fails. The
claim is the revision a record was last offered at: a verdict bumps the
revision, so the record's own next decision retires it. In memory is
enough — AcquireInstanceLock is a flock the kernel drops on process death,
so exactly one connector ever sweeps a ledger.
- The join. Two tests: one drives the sweep, one drives the real ticker with
nothing else offering anything. Deleting the hook fails the second and
leaves the first green.
config_unreadable now retries with no window at all, which is the one change
to the schedule itself. A day is the right bound for a failing server; a
broken connect.json is a local file, and an operator away for a week is
ordinary. no_route, bucket_mismatch and unroutable stay untimed: the first
waits on a decision, the other two on facts no later read changes.
The comments across ledger.go, ledger_events.go, admission/verdict.go and
retraction_test.go that already claimed blocked records were "retained and
retried" are true as of this change, and point at what runs them.
ledger_decisions.go's AuthorizedBlocked still has no caller and now says so.
Adds automatic timed redispatch of eligible blocked connector records.
Changes:
Queries due blocked records within project scope.
Periodically reoffers records with revision-based claims.
Extends config_unreadable retries indefinitely and adds tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
File
Description
internal/connector/retraction_test.go
Updates retry documentation.
internal/connector/ledger.go
Documents timed blocked retries.
internal/connector/ledger_events.go
Queries due blocked records.
internal/connector/ledger_decisions.go
Clarifies authorized retry behavior.
internal/connector/intake.go
Adds periodic retry sweeping and claims.
internal/connector/blocked_retry_test.go
Tests querying, scope, claims, and ticker integration.
Copilot on #770, three findings, each proven with a test that failed first.
A reason that changes gets its own window. blocked_at survives every
blocked-to-blocked verdict, which is right for the person's authorization
that reads it and wrong for a retry window: the reasons carry different ones.
A record that spent a week as the unbounded config_unreadable and then blocked
read_failed was measured against the week-old stamp and got none of the
twenty-four hours read_failed promises — zero attempts, not fewer. That is the
retry stranding the work it exists to recover. events.retry_since is when the
record entered the reason it is blocked on now, and the window counts from it.
The schedule is stored rather than recomputed over the whole table.
events.next_retry_at is what NextBlockedRetry answered when the verdict was
written, NULL when the record is owed no attempt, and indexed. The sweep's
query is a comparison against it, so a record the schedule has finished with
is not read at all, where before every blocked row in the ledger's history was
decoded every minute to be dated and discarded.
Claims are bounded by the live schedule. Each sweep drops the claims of
records the ledger has nothing left to do about, so what is held is the
backlog being worked through rather than one entry per record ever retried —
which after a large outage was a copy of every event id it blocked. The card
justified the in-memory memo on what a crash costs, which is about durability
and says nothing about growth; growth was the actual problem.
A Run no longer inherits the previous Run's claims. resetRunState says nothing
a previous Run decided may leak into the next, and a claim is such a decision:
a Run canceled between the offer and the verdict left the record blocked at
the revision it was claimed at, and the claim suppressed its retry for the
life of the process.
The upgrade puts already-blocked records on the schedule due now, so a ledger
written before this carries no record that silently stopped being retried.
Retry handoff can bypass interval after a concurrent re-decision
internal/connector/intake.go:934
The claim records the revision here, but the queued item carries only the ID. Because basecamp connect redispatch is explicitly supported while the connector runs and performs its own direct re-decision, it can re-block the row at revision N+1 after this offer but before admission takes the ID; admission then loads N+1 and immediately decides it again, bypassing the newly written retry interval. Carry the expected revision through the retry handoff (and skip it when the ledger revision changed), or make the claim durable/atomic with loading.
Copilot on #770, the migration finding and the one it found in code the first
round did not change. Both proven with a test that failed first.
The backfill honoured no throttle. A migration runs once against a real
ledger and leaves the rows behind, so a deadline it ignores is wrong from then
on and no later fix reaches those rows — and the specific harm is retrying
into an active throttle, which is how a rate limit becomes a harder one. It
now writes MAX(decided_at, retry_at): a row whose server deadline is still
ahead waits for it, a spent deadline is due like any other, and a row with no
retry_at at all is due at the next sweep, which is the attempt nothing had
ever computed for it. The migration is not reversible and does not pretend to
be — a newer ledger is refused at open — and it is run-once rather than
idempotent, which the runner guarantees; both are now written above it.
Nothing decides a blocked record before its own schedule says so, whoever
handed the id over. The queue carries an id and not the revision it was
claimed at, and `basecamp connect redispatch` runs beside a live connector
(openConnectLedger takes the instance lock only for an import), so a person
could re-decide a record between the sweep's offer and admission taking it —
and admission would then load the newer revision and decide it at once,
inside the interval that re-decision had just written. LoadUndecided drops
the stale hand-off instead.
A person's redispatch is not a timer and does not wait for one:
authorizeBlocked marks the record due as it authorizes it, so the rerun the
command runs straight afterwards is not refused as early. It writes "due now"
rather than clearing the schedule, so a rerun that never happened — the
command died between the two — stays on the sweep's list instead of being
stranded.
Copilot on #770, both proven with a test that failed first.
The batch counted records read, not records offered. A row already claimed
is due and stays due until admission decides it, so a batch filled with claims
came back full of the same claims tick after tick and never reached the rows
behind — the window starvation the claim exists to prevent, arriving through
the limit instead of through the offer. DueBlockedRetries takes an AfterID and
returns a page; the sweep pages, filters its own claims out of each page, and
stops when it has offered a batch. The claim set lives in memory, where the
filtering is free, rather than being sent to SQLite as an IN list that after
an outage is the whole backlog. The paging walks the due index, not every
blocked row.
Nil meant "no attempt is owed" and was read as permission to run now. A
blocked record with no next_retry_at — an untimed reason, or a window that has
passed — was loaded immediately and on every tick after it. LoadUndecided now
requires a non-nil and due stamp, and authorizeBlocked stamps one
unconditionally rather than preserving nil: without that second half, no_route
and every other untimed reason could not be redispatched at all, which is the
case a person is most likely to redispatch.
The two columns do not mean the same thing by nil, and both now say so where
they are read. retry_at is a server's deadline, and nil is the absence of a
constraint — the ten-minute interval still governs. next_retry_at is the
schedule's answer, and nil is a refusal. The backfill agrees: it never leaves
a scheduled record nil.
ORDER BY id prevents efficient due-time index scanning
internal/connector/ledger_events.go:250
ORDER BY id defeats the new events_next_retry (state, next_retry_at) index: with the existing events_state_id index, SQLite plans this as a scan by (state, id) and filters next_retry_at afterward. Consequently, every minute this can still walk the full retained blocked history before finding a small due page—the scaling problem the persisted due time was meant to remove. Page in (next_retry_at, id) order (and carry both cursor values), or otherwise force a plan that range-scans the due-time index before limiting.
Pruning scans all blocked rows instead of scheduled retries
internal/connector/ledger_events.go:272
This ordering also makes SQLite prefer events_state_id, so pruning scans every blocked row—including untimed and window-expired history—instead of using events_next_retry to visit only the live schedule. Since this query must return the full set, force the due-time index while retaining the documented ID ordering; SQLite can then scan scheduled rows and sort that smaller result.
Contract incorrectly describes retry filtering by reason
internal/connector/admission/commit.go:162
This contract is inaccurate: the ledger queries never read this map or narrow by reason; they filter the next_retry_at value persisted by blockedSchedule. Describing the actual write-time source of truth avoids encouraging a future second reason filter and matches DueBlockedRetries’ documentation.
Copilot on #770, from the Previously missed section of a review whose header
said Findings: None. EXPLAIN QUERY PLAN, before:
DueBlockedRetries SEARCH events USING INDEX events_state_id (state=? AND id>?)
ScheduledBlockedIDs SEARCH events USING INDEX events_state_id (state=?)
state=? AND id>? is every blocked row the ledger has ever retained, walked
once a minute, with next_retry_at applied afterwards — the scan the stored due
time was added to remove, happening anyway. The index was there, the schema
was right, the tests were green, and nothing said the plan ignored all of it.
After:
DueBlockedRetries SEARCH events USING INDEX events_next_retry (state=? AND next_retry_at>? AND next_retry_at<?)
ScheduledBlockedIDs SEARCH events USING COVERING INDEX events_next_retry (state=? AND next_retry_at>?)
No schema change was needed. events.id is INTEGER PRIMARY KEY, so it is the
rowid and every events_next_retry entry is already (state, next_retry_at, id):
ordering by (next_retry_at, id) is the index's own order and sorts nothing.
The due page takes a row-value cursor and the sweep carries both of its
values; the prune query takes the same ordering and its doc no longer promises
an id order its caller does not use. Records now come back longest-overdue
first, which is the better service order as well as the indexed one.
The assertion is the plan. TestTheSweepsQueriesUseTheDueTimeIndex runs EXPLAIN
QUERY PLAN over the SQL both methods build: the due-time index present, the
state/id index absent, nothing sorted. A timing test could not tell the two
apart — at test row counts a full scan is instant — so it would have passed
over the thing it claimed to check, which is this defect in test form.
timedBlockedReasons said the ledger's query narrows by reason. It never has:
the reason reaches the sweep once, through the next_retry_at that
blockedSchedule persists. The comment now says that, and why a second filter
would be a copy of the schedule that could fall out of step with it.
🤖 All three from the Previously missed section of the last review, which had Findings: None in its header. Answering here because none of them carry a review thread.
The index existed and neither query used it
Both correct, and this is the one I would not have caught from the diff: the index is there, the schema is right, the tests are green, and EXPLAIN QUERY PLAN says the queries do something else entirely. Measured, not reasoned about:
BEFORE DueBlockedRetries
SEARCH events USING INDEX events_state_id (state=? AND id>?)
AFTER DueBlockedRetries
SEARCH events USING INDEX events_next_retry (state=? AND next_retry_at>? AND next_retry_at<?)
BEFORE ScheduledBlockedIDs
SEARCH events USING INDEX events_state_id (state=?)
AFTER ScheduledBlockedIDs
SEARCH events USING COVERING INDEX events_next_retry (state=? AND next_retry_at>?)
state=? AND id>? is every blocked row the ledger has ever retained, walked once a minute, with next_retry_at applied afterwards — exactly the scan the stored due time was added to remove.
The fix is the first option you name, and it needs no schema change.events.id is INTEGER PRIMARY KEY, so it is the rowid and every events_next_retry entry is already (state, next_retry_at, id). Ordering by (next_retry_at, id) is therefore the index's own order:
DueBlockedRetries pages on a row-value cursor — (next_retry_at, id) > (?, ?) with ORDER BY next_retry_at, id — and the sweep carries both values. It now returns the longest-overdue first, which is also the better service order.
ScheduledBlockedIDs takes the same ordering. I did not use INDEXED BY: with it the plan keeps the id ordering but adds USE TEMP B-TREE FOR ORDER BY, and this result set is read as a set by the caller, so the order is the index's to choose. Saying so is cheaper than sorting. The doc comment now says that rather than promising id order.
Neither plan has a temp b-tree.
The assertion is the plan, not a timing.TestTheSweepsQueriesUseTheDueTimeIndex runs EXPLAIN QUERY PLAN over the SQL both methods build and asserts events_next_retry is present, events_state_id is absent, and nothing is sorted. At test row counts a full scan is instant, so a timing test would pass over the very thing it claimed to check — which is the defect this finding is, in test form.
The contract described a filter that does not exist
Fixed. timedBlockedReasons has exactly one reader, NextBlockedRetry, and that is the whole of how a reason reaches the sweep: the answer is computed once when the verdict is written and stored on the row. The ledger's queries never see a reason. The comment now says that, and says why a second filter down there would be a copy of the schedule that could fall out of step with it — which is the thing it was inviting.
That is the fourth comment on this PR describing behaviour the code did not have, and three of the four were mine.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Each sweep performs unbounded work over the complete scheduled backlog despite its configured batch limit.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Unbounded retry pruning materializes entire backlog each sweep
internal/connector/intake.go:936
This makes every one-minute sweep materialize the entire scheduled backlog before offering at most 100 records. In particular, the first sweep pays this unbounded query even when retriedBlocked is empty, and after a large outage each later sweep allocates an ID slice/map proportional to all blocked retries, defeating the batch's bounded-work purpose and delaying stranded/loss repair in the same goroutine. Prune by checking only the IDs currently present in the claim map (in chunks), and skip pruning entirely when there are no claims.
🤖 Previously missed (1) — "Unbounded retry pruning materializes entire backlog each sweep", internal/connector/intake.go:936. Correct, and fixed in
the head above.
Right on every count, including the one I had written a docstring to defend. ScheduledBlockedIDs said of itself that it was "deliberately unlimited",
because a short answer would prune claims that were still live and offer their records twice. That reasoning is sound and it answers a question the sweep
never asks. The sweep does not want the live backlog; it wants to know which of its own claims can still be offered. Those have the same answer for the
claims held, and the costs are not close: after an outage the schedule holds every record the outage blocked, and that was being read once a minute, in the
goroutine the stranded and loss repair share, to decide the fate of at most a batch of ids. The empty-claims case paid it too, which is the first sweep of
every run.
So the read is now of the claims. StillScheduledBlockedIDs(ctx, scope, ids) returns the subset of ids still scheduled, chunked at 400 placeholders with
the answer being the union — a dropped or shortened chunk would read as "no longer scheduled", so a failed chunk fails the call rather than pruning on a
partial answer. The sweep asks nothing when it holds nothing.
The safety you were protecting is stronger now, not weaker. Pruning considers only the ids it asked about. The old prune dropped every claim the answer
did not name, so a claim taken while the read was in flight — never put to the ledger, so never in its answer — was dropped, and its record offered a second
time. That was a real latent double-offer in the code you were reviewing, and bounding the question is what closes it. TestPruningNeverDropsAClaimItDidNot AskAbout holds it.
On the assertion, since this is the fourth round this mechanism has found a new way to cost the backlog instead of the batch: the answer was never wrong,
which is exactly why nothing caught it. Same shape as the index finding last round — schema right, results right, work quadratic, every test green. So the
ledger counts its schedule reads and TestPruningAsksAboutTheClaimsHeldAndNotTheBacklog asserts a sweep holding no claims makes none. Both tests were run
against the old shape first and fail there, on the two assertions above and not on collateral.
The index-plan assertion for the pruning query changed with it: it is an id IN (...) lookup now, so the rowid finds it directly and asserting events_next_retry would be asserting the wrong plan. It asserts no SCAN events and no events_state_id — the claims are looked up, never scanned for.
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
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.
A blocked record that says it retries on a timer waits for a person instead.
admission.NextBlockedRetrynames the reasons that should come round again, computes when each is next due, and nothing calls it. The only path that re-offers a blocked record isbasecamp connect redispatch <id>, one id at a time — so a read that failed during an outage, a throttle, and aconnect.jsonthat could not be read all sit until somebody notices and redispatches each one. Repairing the file, or the outage ending, clears nothing by itself.Several comments across the connector say otherwise — "retained and retried", "recovered on the blocked schedule". They were left standing rather than corrected, because building this is what makes them true. This builds it, and the ones it does not make true are corrected here.
Originally tracked in A blocked record's timed retry is described but never runs. The schedule itself arrived in Decide which feed events are the agent's business: admission with no caller, and a first attempt at the caller was built and taken back out of The connector runs where it is started: no directory is a project's over three review rounds.
The shape
The intake sweep that already re-offers stranded events and open losses now also asks the ledger which blocked records are due and offers those. The schedule stays where it was —
NextBlockedRetryis still the one place that says when a record is next owed an attempt; the ledger's query only narrows to the rows it could possibly say yes about, and the sweep only moves them.Three things the earlier attempt was taken out over, each addressed here rather than assumed away.
Scope. The query takes the run's
--projectbuckets and will not return a record outside them.out_of_scopeis a terminal discard and the projects a run leaves out are another run's to dispatch, so a retry that crossed the scope would cause the permanent loss it exists to prevent. Every other terminal verdict a re-offered record can reach —stale,not_addressed,untrusted_performer,agent_authored— is the verdict a fresh event would get, and is correct.The claim.
Queue.Offerdoes not deduplicate, so a due row is claimed before it is offered and the claim is given back if the offer fails. Without one the same rows requeue on every tick while the rows behind them in the window starve, and a second copy can decide a record the moment the first re-blocked it, bypassing the interval. The claim is a map from record to the revision it was last offered at: a verdict bumps the revision, so a record's own next decision retires its claim. In memory is enough —AcquireInstanceLockis a flock the kernel drops on process death, so exactly one connector ever sweeps a ledger, and a crash costs at most one extra decision, which admission's revision guard already makes harmless.The join. The removed version's end-to-end test called the query and the queue itself, so it stayed green with the periodic hook deleted. There are two tests here: one drives the sweep directly, one drives the real ticker with nothing else offering anything. Deleting the hook was checked against both — the ticker test fails, the sweep test stays green.
Which reasons this turns on, which it does not
Wiring this enables automatic re-decision for every reason the schedule names. That is a feature, not a defect fix, so it is worth saying plainly what it decides now:
read_failed— retried, 24h. The case the schedule exists for. It covers 401/403/404 too, which heal when access is granted or a token is refreshed; the window is what stops a deleted recording being re-read forever.read_unresolved— retried, 24h. A chat line under no visible Campfire; visibility changes when a project is granted.delta_unverified— retried, 24h. The assignment event fell outside the events read's bound, and a later read can find it.trust_unverified— retried, 24h. Time is the entire remedy: the only listing at hand predates the event and a fresh one was read too recently to read again.throttled— retried, 24h, never before the server's own deadline.config_unreadable— retried, with no window. This is the one change to the schedule itself. A day is the right bound for a failing server: after one, it is not coming back on its own and a person should see the record. A brokenconnect.jsonis a local file, and an operator away for a week is ordinary — giving up there strands the work silently.no_route,bucket_mismatch,unroutable— not retried, unchanged. The first waits on the operator serving the project, which is a decision rather than a delay; the other two are facts about the pointer that no later read changes.Nothing wanted excluding, so the reason set lives in the query rather than in the scheduler: one list, shared by the function that dates a record and the query that finds it, with a test that they cannot drift apart.
A due row the schedule has finished with stays blocked forever, so the query pages past those rather than cutting the scan off with a
LIMIT— otherwise they would hold a place in every sweep's window for good.