feat(orchestrator): distinguish a mount serving empty trees from one timing out (#351) - #363
feat(orchestrator): distinguish a mount serving empty trees from one timing out (#351)#363khaliqgant wants to merge 1 commit into
Conversation
…timing out (#351) Follow-up to #354, which shipped without a requirement asked for twice on #351. #354 made a hung relayfile read loud: a per-call deadline, a named `RelayfileOperationTimeoutError`, a rising `consecutiveFailures`. It left the other half open. A mount that starts answering reads with *nothing* instead of hanging raises no timeout, no failure and no `lastError` — the sweep completes `healthy` and dispatches nothing, and on every field the surface publishes that is indistinguishable from a workspace with no ready work. #355's `candidates: 0` cannot separate them either; it is the same observation one layer down. So a sweep now reports both the tree reads the backend served and how many came back empty, on `readinessReconcile` beside `candidates` and as the `relayfileEmptyTreeReads` counter. Deliberately a pair, not a count. The first version of this was a bare empty-read counter and its own control test caught it firing on a healthy sweep: discovery lists two path forms per repo and only one of them exists, so an empty read is ordinary. What is not ordinary is `emptyTreeReads === treeReads` with `treeReads > 0` — the mount served nothing at all. `candidates: 0, treeReads: 3, emptyTreeReads: 1` is an empty workspace; `emptyTreeReads: 3` is a silent mount. Also closes two smaller gaps from the #351 review that #354 answered by inference rather than assertion: - The discovery lease released by the unwind is now asserted directly, by taking it with a different owner after the abort. `#runOnceWithDiscoveryFence`'s `finally` is the only `releaseDiscoverySweep` call site and `stop()` has none, so a lease free there was freed by the aborted sweep. - `#runOnceInFlight` clearing is now asserted by requiring a SECOND independent failure while the dependency is still hung. Had the field still held the wedged promise, cycle two would have coalesced onto it and never settled, pinning `consecutiveFailures` at 1. The new fixtures get an isolated mkdtemp registry, heartbeat and state store. The config default points every instance at one shared path, and without that isolation these two live-mode factories leaked rows into whatever ran next -- two unrelated Slack/babysitter tests failed downstream. Caught by comparing against a clean base rather than assuming an unrelated failure was the substrate. Verified by ablation: disabling the empty-read increment fails the new fault test on the count, and the control fails if the signal is reduced to a bare count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai review Requested for exact head |
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 379456a701
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| treeReads: this.#readinessReconcileLastSweep.treeReads, | ||
| emptyTreeReads: this.#readinessReconcileLastSweep.emptyTreeReads, |
There was a problem hiding this comment.
On deployed instances using the documented unauthenticated /healthz route, these fields never appear: publicHealthFromHeartbeat() passes readiness data through sweepOutcome(), whose whitelist and FactoryPublicReadinessReconcileHealth omit both fields, and normalizePublicHealth() consequently strips them as well. Thus the default factory diagnose --deployed path still cannot distinguish an empty mount from an empty workspace, even though the internal status() object contains the new values; carry and validate both counts through the public-health projection.
Useful? React with 👍 / 👎.
| if (count !== undefined) { | ||
| this.#discoverySweepTreeReads += 1 | ||
| if (count === 0) { | ||
| this.#increment('relayfileEmptyTreeReads') | ||
| this.#discoverySweepEmptyTreeReads += 1 |
There was a problem hiding this comment.
In live mode, event drains and completion timers can call #listRelayfileTree concurrently with runOnce(), and this unconditional increment counts every successful list operation in the process rather than only calls issued by the readiness sweep. A populated PR/Slack lookup overlapping an otherwise all-empty discovery pass makes emptyTreeReads < treeReads and masks the silent-mount signal; conversely, unrelated empty lookups can distort the ratio. Track these counts in sweep-local context or increment them only at enumeration call sites.
Useful? React with 👍 / 👎.
|
CI green on all five jobs (run 32698619106):
Ready to merge. The one open decision is |
|
Both threads block, and the P1 is the third occurrence of the same systemic defect. Worth naming it as such. P1 at
|
1 participant
Follow-up to #354. Closes a requirement asked for twice on #351 that #354 shipped without, plus two review points it answered by inference rather than assertion.
The gap
#354 made a hung relayfile read loud — per-call deadline, named
RelayfileOperationTimeoutError, risingconsecutiveFailures. It left the other half open, and you named it explicitly:A mount answering reads with nothing raises no timeout, no failure and no
lastError. The sweep completeshealthyand dispatches nothing. #355'scandidates: 0cannot separate that from a workspace with no ready work — it is the same blind spot one layer down.The signal, and why it is a pair
readinessReconcilenow publishestreeReadsandemptyTreeReadsbesidecandidates, plus arelayfileEmptyTreeReadscounter.Deliberately two numbers, not one. My first version was a bare empty-read counter, and its own control test caught it firing on a perfectly healthy sweep: discovery lists two path forms per repo and only one of them exists, so an empty read is ordinary. A counter that alarms on every normal sweep is worse than no counter. What is not ordinary is the ratio:
candidates: 0, treeReads: 3, emptyTreeReads: 1candidates: 0, treeReads: 3, emptyTreeReads: 3Both published unconditionally, including zeroes, for the same reason
dispatchFailuresis: the comparison is the signal, and an omitted zero would make "served real content" indistinguishable from "field not reported".The two assertion gaps from your #351 review
You asked me to assert these; #354 inferred them from "the next cycle ran".
#runOnceWithDiscoveryFence'sfinallyis the onlyreleaseDiscoverySweepcall site andstop()has none, so a lease free there was freed by the aborted sweep.#runOnceInFlightcleared — now asserted by requiring a second independent failure while the dependency is still hung. Had the field still held the wedged promise, cycle two would have coalesced onto it and never settled, pinningconsecutiveFailuresat 1 forever.Still open, your call
lastErrornames operation and phase but not path/prefix, which you asked for. I left it out becauselastErroris persisted and returned fromstatus(), and #297's precedent builds that string only from closed-set tokens — a workspace path is dependency-shaped text on an operator surface. The path is in the[factory] relayfile operation failedlog line. Say the word and I will add it; I should have flagged the deviation on #354 rather than making it silently.Verification
src/orchestrator/factory.test.ts: 586/586 locally. Ablation: disabling the empty-read increment fails the fault test on the count; reducing the signal to a bare count fails the control.One thing worth flagging — two unrelated tests failed downstream of my first draft and they were mine, not the substrate. The new live-mode fixtures wrote to the config's shared default registry path and leaked rows into whatever ran next, breaking a Slack-coalescing and a babysitter test. I caught it by running those two against a clean base rather than assuming an unrelated failure was #342. They now get an isolated
mkdtempregistry, heartbeat and state store.🤖 Generated with Claude Code
Summary by cubic
Distinguishes an empty-serving mount from a timeout in orchestrator readiness (Linear 351). Previously, a mount that always returned empty trees looked healthy and identical to a workspace with no ready work; now the status exposes why
candidates: 0occurs.treeReadsandemptyTreeReadstoreadinessReconcileandIterationReport. Counts are per-sweep, read before reset, and published even when zero. IncrementsrelayfileEmptyTreeReads.treeReads > 0andemptyTreeReads === treeReads.#runOnceInFlightclears (proven by a second independent failure).docs/deployed-diagnostics.mdto document interpretation.mkdtempregistry/heartbeat to prevent cross-test leakage.Rollout
status()will see optionaltreeReadsandemptyTreeReads.treeReads > 0 && emptyTreeReads === treeReads && candidates === 0. Consider trackingrelayfileEmptyTreeReads.Written for commit 379456a. Summary will update on new commits.