test: register fixture writer capability in subc bridge tests - #194
Conversation
29fa3f7 to
3ac5c3b
Compare
|
Pushed a manifest refresh — the two red jobs were my omission, not the fix. The legacy-vocabulary allowlist pins occurrences by Regenerated with
One thing I deliberately left out. Running the regeneration with
|
|
Hold off merging this — the patch has a side effect I hadn't accounted for, and it's mine, not a flake.
I pulled that test's Windows result across the last ten runs:
2/2 on mine, 0/8 on yours. On Working hypothesis, not yet confirmed: If that holds, the question is whether to scope the registration to the one test that needs it, or whether the routebind assertion was calibrated against a configure that did nothing — in which case the more interesting possibility is that a real prioritization weakness under load was previously masked. I don't want to guess between those on your repo, so I'm digging before I push anything else. I can't reproduce it locally (Linux), so I'm working from the code and will be explicit about what's inference. Will report back either way — if it turns out the right fix is narrower, I'll push that; if it points at something in the product, you'll want to know regardless of this PR. |
|
Root-caused, and it's narrower than I feared — no product defect, and my callgraph guess was wrong. The routebind test disables the callgraph store ( Which is exactly the window
So this is a measurement-boundary regression against that stated invariant, not a starvation defect. The ack hasn't been generated yet when the 799 frames are counted — the priority mechanism never gets a chance to fail. And given Fixing it by keeping the registration out of the timed window rather than by touching the bound or the marker. Will push once the full integration binary is green locally — running the whole binary this time, not just the target test, since a shared fixture is exactly where that shortcut bites. To be explicit about the limits: I'm on Linux and the failure is Windows-only, so nothing I run locally can prove it fixed. Your CI is the oracle. The Linux runs only establish non-regression. |
3ac5c3b to
4ba673f
Compare
|
Pushed the narrower fix. The registration now happens in let canonical_callgraph_root = std::fs::canonicalize(callgraph_root.path())
.unwrap_or_else(|_| callgraph_root.path().to_path_buf());
// Register the fixture's writer capability before transport timing begins.
aft::root_cache::configure_artifact_access(
&canonical_callgraph_root,
&aft::search_index::artifact_cache_key(&canonical_callgraph_root),
false,
);and it's gone from Two alternatives I rejected. Moving the routebind test's completion marker would work but changes measurement semantics other tests depend on, for a problem my patch introduced. Substituting a cheaper key to dodge the git subprocess would let the fixture register a different key than production computes — granting capability for the wrong root and masking future breakage. The fixture should use production's real key; it just shouldn't compute it inside a timed path. Local, on Linux: Ran the whole binary this time rather than just the target — a shared fixture is exactly where that shortcut bites, as this round demonstrated. The 26 are the two Manifests regenerated again since the line count shifted by 1: allowlist still 678 entries, the 16 Standing caveat: I'm on Linux and the failure is Windows-only. Removing the git probe from the timed window addresses the mechanism structurally, but only your CI can confirm it. |
|
Green — 14/14. Both tests pass on the Windows job that was the oracle here: The one I couldn't verify locally is confirmed: hoisting the registration out of the configure path fixed the routebind regression, and the callgraph test still gets its writer capability. Ready to merge whenever you are. It should take |
Fixes the last failure keeping
mainred (#193). Same class and same fix asc718f898and89f8d5ce— one more fixture that never registered its root.configure_bridge_contextinsubc_bridge_test.rsis the tests' stand-in for configure. It sets the config, the canonical cache root, and the cache role, but never callsconfigure_artifact_accessthe way real configure does atcommands/configure.rs:2225-2229. That was harmless untilbe76cf8bmade unregistered roots fail closed:ArtifactAccess::for_rootnow defaults to borrow-only and writer-shaped cold-build paths returnUnavailablerather than falling back to a borrower.So
ensure_built_with_lease_chunkednever gets a lease, the background ensure settles without publishing,.currentis never written, and every retry comes backcallgraph_buildinguntil the 90s poll gives up:The
fake daemon joinspanic at:1483is the daemon thread joining on the already-panicked poller, not a second fault.Verification
Red → green → red → green on the targeted test, so the pass isn't a flake getting lucky:
1 passed; 0 failed0 passed; 1 failed1 passed; 0 failedThe 0.72s is the useful number. The lease is granted and the store publishes immediately, rather than the poll happening to win a race — a timing fix would still take most of the budget.
I also checked the rest of the file for the same omission:
configure_bridge_contextis the only helper touchingset_cache_role/set_canonical_cache_rootwithout a registration, and it has a single call site.Not timing and not a product defect. The lease is denied before any build or publication begins, and production configure registers correctly — this is only the fixture drifting behind the new gate.
Ran the full integration binary too. Beyond the two sandbox tests that fail on my box for local PATH reasons, I get 24
callgraph_test::*failures reportingcallgraph_unavailable— those are an artifact of my running from a linked worktree (.git/worktrees/...), not from this change;configure_bridge_contextis defined and called only insubc_bridge_test.rs, so it can't reach them. They should not appear on a normal checkout, but flagging it so the result isn't a surprise if you see it.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Register the writer capability for the callgraph test cache root in
subc_bridge_test.rs, matching production. Canonicalize the root and callaft::root_cache::configure_artifact_accessso cold builds get a lease, publish immediately, and thecallgraph_buildingretries go away.Also regenerates the v0.49 agent-surface manifest and legacy vocabulary allowlist to update the source commit and hashes.
Written for commit 4ba673f. Summary will update on new commits.
Greptile Summary
The PR registers writer capability for the temporary callgraph root used by the subc bridge fixture, aligning the test setup with production configuration.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (3): Last reviewed commit: "test: register fixture writer capability..." | Re-trigger Greptile