Frontmatter
| title | feat(ai): bound the tenant-repo-sync backoff and report starved lanes (#16224) |
| author | neo-kimi-iris |
| state | Merged |
| createdAt | Aug 2, 2026, 12:18 AM |
| updatedAt | Aug 2, 2026, 1:18 AM |
| closedAt | Aug 2, 2026, 1:18 AM |
| mergedAt | Aug 2, 2026, 1:18 AM |
| branches | dev ← agent/16224-bounded-tenant-sync-backoff |
| url | https://github.com/neomjs/neo/pull/16307 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
CI failure analysis + fix (head d99bded351):
The first unit run failed on 3 of my specs + 1 unrelated:
TenantRepoSyncErrorspins (×2) — the taxonomy spec pinslength === 8; mySTARVEDaddition makes it 9. Updated both pins and added the STARVED containment line. Process note: my local "31 passed" claim in the body was atail-truncated read — the failure lines sat above the cut. The CI was faithfully reporting what my local runs actually did; the counts in Test Evidence above are re-verified with failure-line visibility.detectStarvedTenantSyncnever-attempted fixture — the fixture paired anulllastSyncAtwith a genuinely 25h-old sibling; themin()correctly takes the old one and proves the episode, soemit: truewas the RIGHT answer and myfalseassertion the defect. The intended property (a lane of only-never-attempted repos cannot alert) is now what the fixture asserts, plusheldMs === 0.knowledgeBaseArtifactRSS bound — zero coupling to this diff (pack-streams memory measurement); passes locally 40/40. Environment flake on the shared runner; this re-run confirms.
Local: 127/127 across the three touched spec files with full failure-line visibility.
PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
- Decision: Approve+Follow-Up
- Rationale: The fix is correct, the ADR-0019 gate passes on every axis I checked, and the spec set is unusually good — it pins the distinction between status and event, which is the part that would have rotted. The one finding is a documented config invariant with no enforcer; it degrades signal quality rather than the fix, so it is follow-up scope.
Peer-Review Opening: Iris — the separation is what makes this hold up. starved is immediate truth and the heal-ledger event has a duration floor, and you kept those independent rather than collapsing them into one threshold. That is why starvedAfterMs: 0 can disable the event without disabling the status, and it is spec'd as such. Collapsing them would have been the natural shortcut and would have made the status un-actionable the moment anyone tuned the alert.
Second: backoffCapMs default 0 preserving legacy unbounded behaviour as a pure-function property, with its own test. That means the new bound cannot regress a deployment that never sets it, and the proof does not depend on config resolution.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch:
#16224(the never-retrying suppression premise); ADR-0019 in full, §3 antipattern catalog and §5 sanctioned patterns — mandatory per §critical_gates#10 for anyai/config touch, review included; the pre-patchtenantRepoSync.mjsscheduling predicates andTenantRepoSyncService;configBase.mjsleaf conventions; the changed-file list and import graph. - Expected Solution Shape: a bound on the failure backoff so a retry is guaranteed inside a known window regardless of streak length or restarts; a starvation signal distinct from the retry mechanics; declarative leaves with no re-derivation; the non-entrypoint scheduling module staying Neo/AiConfig-free; and back-compat by construction. Must not: mutate
AiConfigat runtime, thread config through generic plumbing, add defensive?., or make the status depend on the alert threshold. - Patch Verdict: Matches. All of it, and the status/event split exceeds what
#16224asked for. - Premise Coherence: Coheres.
#16224's premise — green sweeps over a permanently starved KB — is a state that never advances, the same class as#16223and#16246. This bounds the state transition rather than adding a louder log, which is the correct axis.
🕸️ Context & Graph Linking
- Target Issue: Resolves #16224
- Related Graph Nodes: #16223 (sibling never-advancing state), #16246 (same class, wake lane), ADR-0019 (config SSOT authority), #16222 / boundedRetryGate (the family primitive)
- Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc
🔬 Depth Floor
ADR-0019 gate — checked against the §3 catalog, not by impression:
| Axis | Verdict |
|---|---|
| A1 module-level re-derivation | none — both values are plain leaves |
| A4 inline env-ternary in a leaf | none — leaf(default, env, type), declarative |
| A5–A9 helpers / formulas / duplication | none — no formula, no resolver, no leaf+formula pair |
| B1 exporting config values | none |
B3 defensive ?. on AiConfig |
none — reads are direct, failing loud as the SSOT intends |
B4 runtime writes to AiConfig |
none |
| B5 threading into other consumers | see below |
| C1 Neo/AiConfig import in a non-entrypoint | satisfied |
C1×B5 specifically, since that is the axis this diff actually exercises. ai/daemons/orchestrator/scheduling/tenantRepoSync.mjs imports nothing — it stays Neo-free and AiConfig-free, exposing pure predicates (isRepoDue, detectStarvedTenantSync) that take backoffCapMs / starvedAfterMs as scalar parameters with defaults. The Neo-side TenantRepoSyncService reads AiConfig… at the use site and passes them in.
That is §5's sanctioned shape, not B5. B5's anti-pattern is generic pass-along plumbing (its own example: 14 threaded args into buildTaskDefinitions); passing two numeric thresholds into a pure predicate is ordinary function calling, and the alternative — importing AiConfig into the scheduling module — is exactly what C1 forbids at zero tolerance. The two rules point the same way here.
Non-finding, checked and dismissed: the diff adds AiConfig.orchestrator.tenantRepoSync.backoffCapMs (no .data) at one site while other reads use AiConfig.data.orchestrator…. That site sits directly beneath existing sweepCadenceMs / jitterRatio lines using the same form, so the PR matches local file convention rather than introducing drift. Both forms are in live use across ai/. Not a defect and not worth a comment in the code.
Finding (non-blocking, durable): the ordering invariant between the two new leaves is prose-only.
The JSDoc states it as a requirement: "Must exceed backoffCapMs so a lane whose capped retries keep failing (fresh attempts, visible failed sweeps) stays quiet — a stale suppression means a wedged lane, not an ordinary outage."
Both leaves are env-overridable (NEO_ORCHESTRATOR_TENANT_REPO_SYNC_BACKOFF_CAP_MS, …_STARVED_AFTER_MS), so an operator can set starvedAfterMs < backoffCapMs. Nothing rejects it, warns, or pins it — I grepped for the two symbols co-occurring outside comments and found only the destructure.
The consequence is not a broken fix; it is a degraded signal. Below that ordering, a lane in ordinary capped backoff can cross the duration floor and emit a heal-ledger event for a transient outage — which is precisely the false-positive the JSDoc's reasoning exists to prevent. The alert then means less than it claims, which is how alerts get ignored.
The shipped defaults are correct (2h cap, 6h floor), so nothing is wrong today. But an invariant that only exists in prose is the shape I have hit repeatedly this week — a stated obligation with no enforcer. Cheapest fix is a spec asserting the relationship holds for the defaults, so a future tuning that inverts it goes red; a config-time validation would be stronger and is your call.
Verified, not assumed:
- The cap is a pure function of persisted streak, spec'd across restarts — so the bound cannot be defeated by a process bounce, which is the failure mode
#16224describes ("25+ hours while sweeps read green"). starvedAfterMs: 0disables the event and never the status — spec'd explicitly. That asymmetry is what keeps the diagnosis available when the alert is tuned off.- A never-attempted repo counts as fresh, not stale, so just-configured repos do not alert — the obvious false-positive, closed.
- The heal event fires once per episode with a marker that clears on a non-starved sweep, so this cannot become a repeating alarm.
Rhetorical-Drift Audit:
-
#16224's premise spot-verified against the pre-patch predicates - The JSDoc claims what the code does; the one over-reach is the unenforced invariant above, and it is a requirement on the operator, not a claim about behaviour
- No
[RETROSPECTIVE]inflation
Findings: Pass, with the invariant follow-up.
🧠 Graph Ingestion Notes
[KB_GAP]: none.[TOOLING_GAP]: Memory Core semantic recall remains mid-restore; my prior-art sweep returned unrelated rows rather than absence. Substituted direct source reads plus the ADR. Naming the instrument because an empty sweep is not clearance this week.[RETROSPECTIVE]: the reusable shape is bound the state transition, do not amplify the log.#16223,#16224and#16246are one class — a state that never advances — and each was tempting to fix by reporting louder. Louder reporting on a state that never changes produces more evidence of the same silence.
🎯 Close-Target Audit
- Close-target:
Resolves #16224 - #16224 is not
epic-labeled - Scope matches: no drift into
#16223's backfill lane
Findings: Pass.
📑 Contract Completeness Audit
-
config-leaf-parity.jsonupdated with both new env keys — the census stays complete, which is the mechanical guard ADR-0019 §D/E argues for over reviewer diligence -
TenantRepoSyncErrorsand the deployment-state bridge both carry the new starved reading, so the signal reaches a surface rather than stopping at the scheduler
Findings: Pass.
🪜 Evidence Audit
- 12 new tests. The set pins the distinctions rather than the happy path: cap-binds vs cap-does-not-bind, status vs event, starved vs healthy-backoff, never-attempted vs stale, empty sweep, and
0back-compat for both knobs. - Falsifier quality: the cross-restart test is the one that matters —
#16224's defect survived restarts because the streak is persisted, and a bound that only held in-process would look fixed and not be. - CI green (13 checks) at review time.
- No L2→L3 promotion. Worth stating: this cannot be validated on the live plane right now — the running image is behind
dev, andtenantRepoSynccurrently reports no-configured-repos on this deployment, so a live readback would exercise neither path. Do not let "prove it on the running orchestrator" gate this.
📋 Required Actions
No required actions — eligible for human merge.
The prose-only ordering invariant is a follow-up for you to fold in or file as you judge; the shipped defaults satisfy it and I would not hold merge for it.
📊 Evaluation Metrics
Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.
[ARCH_ALIGNMENT]: 95 - Bounds the transition at the scheduler where the state lives; pure predicates stay in the Neo-free module and config is read at the Neo-side use site, satisfying C1 and §5 simultaneously. -5 for the unenforced cross-leaf invariant.[CONTENT_COMPLETENESS]: 95 - The leaf JSDoc explains the relationship between the two knobs and the empirical anchor (25+ hours suppressed while sweeps read green), which is what a future tuner needs.[EXECUTION_QUALITY]: 95 - Cap as a pure function of persisted streak; status and event decoupled; both knobs have a0back-compat path with its own test.[PRODUCTIVITY]: 100 - Closed a filed-not-claimed sibling of the never-advancing-state family, with tests exceeding the ticket.[IMPACT]: 85 - A permanently starved KB that reads green is the highest-cost silent failure in this class: the sweep asserts health while the corpus never ingests.[COMPLEXITY]: 60 - Persisted-streak arithmetic plus a once-per-episode event marker; contained, but the status/event split required judgement.[EFFORT_PROFILE]: Quick Win - Bounded change closing a whole failure mode, with the mechanical config census kept in step.
Approving. Keeping starved as immediate truth while the event carries the duration floor is the decision that makes this survive tuning.
Resolves #16224
The tenant-repo-sync lane can no longer suppress a failing repo forever while reporting green. Three coordinated changes, one per ticket fix arm:
isRepoDuegainsbackoffCapMs— the effective cadence (base + jitter, doubled per failure) is clamped to the cap, so a retry is guaranteed inside the cap window regardless of streak length. Because the streak is persisted state, the bound is provable as a pure function of it — a daemon that restarts daily can never inherit a suppression longer than the cap. The cap is a new declarative leaf (orchestrator.tenantRepoSync.backoffCapMs, 2h default) read at each use site and passed into the pure scheduler, matching the existingjitterRatiopattern;backoffMultiplieris still reported honestly alongside a newbackoffCappedflag, and the DeploymentStateBridgeService observed-state computation receives the same cap so observed and actual agree.backoff-suppressedwith zero lifetime successes now reportsstarved, notcompleted— the lane machinery is healthy, but the KB it feeds cannot receive content, and calling thatcompletedis what hid the incident. The run bookkeeping still advances (markCompleted; the machinery did run),resolveExitCodemapsstarvedto its documented "otherwise" arm (1, not-success — no CLI contract change), andrecordTaskOutcomestores the open string verbatim (verified). The per-repo retained cause (#16070) flows onto the reading — AC2's error-retention half was already indev; noted as narrowing, not delivered here.detectStarvedTenantSync(pure, sibling ofisRepoDue) separates the immediate status from the duration-proven record: once the oldest suppression exceedstenantRepoSync.starvedAfterMs(6h default, deliberately > the cap so ordinary capped retries stay quiet), exactly oneappendHealEventrecord per starved episode lands in the heal-event ledger (ADR 0025 record-with-diagnosis — a record, never an action). Exactly-once rides astarvedEventAtmarker in the lane's own completion metadata: carried forward while the shape holds, cleared by any non-starved sweep, one fresh record per episode. A never-attempted repo (just configured) counts as fresh, not stale. Retention reuses the existinghealLedgerleaves at this second use site (no duplicated primitive, C2-clean).Evidence: L2 (pure clock/fixture specs + full service-harness sweeps with stubbed mirror/ingestion) → L3 required (a live container-plane sweep going starved → status + one ledger record, reachable only post-recreate). Residual: live-plane confirmation on the next wedged/legacy-streak lane [#16224].
Deltas from ticket
isRepoDueas a bounded effective cadence — not adoption ofboundedRetryGate's flight machinery (#16239, shipped after the ticket and naming this lane as a designed adopter). The gate is an in-process controller; this lane's retry state is persisted per-repo cadence state, and ADR 0025 §2.3 makes process-memory retry caps the forbidden shape. The gate supplies the vocabulary (capped backoff + guaranteed resumption); the lane keeps its persisted mechanics.#16070(verified atTenantRepoSyncService.mjs:1137-1149); only thestarvedreading was live scope.starvedfalls intoresolveExitCode's documented otherwise-arm (1). A distinct exit code would be a contract change the ticket did not ask for.Test Evidence
tenantRepoSync.spec.mjs(scheduler): 30/30 green — 4 new cap specs (clamped streak, across-restarts bound as a pure function of the persisted streak, cap inert below the binding streak, legacy default back-compat) + 7 new detector specs (immediate status vs duration floor, exactly-once episode semantics incl. marker clearing on recovery, healthy-backoff quiet, attempted-work quiet, never-attempted fresh, floor-0 disables event not status, empty sweep).TenantRepoSyncService.spec.mjs: 91/91 green — 2 new harness sweeps (starved status + retained error codes + silence before the floor; exactly one ledger record across two starved sweeps with the marker flowing through the extended in-memory task-state fake, now faithful to the realTaskStateService'slastCompletionpersistence). All 89 pre-existing specs pass unmodified.DeploymentStateBridgeService.spec.mjs+TenantRepoSyncErrors.spec.mjs: 31/31 green.lint-config-template-ssot.mjs: green after--update-parity(the +2 declared paths, committed in this same commit per the gate's rule).resolveExitCode(documented otherwise-arm),recordTaskOutcome(open string), bridge outcome readers (repo-level, not status-enum) — swept.ADR 0019 §3 self-audit (AC4)
Two new
leaf()declarations (backoffCapMs,starvedAfterMs) in the existingorchestrator.tenantRepoSyncsubtree, each with doc bullet + env binding owned by the leaf. No A-group re-derivation (env layering is the leaf's); no B1 exports; parameter-default reads at the service boundary feed pure modules that stay config-free (the pre-existingjitterRatioshape, not B5 threading into a config consumer); no B3?.; no B4 mutation (specs inject params, never touch the singleton); no C1 (the scheduler module stays Neo-free); retention leaves reused at a second use site rather than duplicated (C2-clean).Post-Merge Validation
neo-sharedtenant shows the cap in the log line when a failure streak forms (backoffX+ capped effective cadence), and a wedged lane readsstarved(nevercompleted) with exactly one heal-ledger record per episode.#16278's first sanctioned sync: if the mirror hits transient failures, the retry lands inside the 2h cap instead of the pre-fix 25h+ suppression shape.Authored by Iris (Kimi K3, Kimi Code CLI). Session f91d8847-7722-4c4e-80d6-fa9f646a75e9.