-
Notifications
You must be signed in to change notification settings - Fork 0
test(release): authenticate qualification evidence #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2cdbd7a
8d7c12d
fe8ee1d
a4344f6
348a1f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -895,6 +895,11 @@ export class SynapseEmbeddingProvider implements EmbeddingProvider { | |
| } | ||
| for (let start = 0; start < items.length; ) { | ||
| if (signal?.aborted || this.permanentFailure) break; | ||
| // A `module_restarted` failure on an earlier page invalidated the | ||
| // compatible daemon identity. Re-run the full initialization so | ||
| // the remaining pages only proceed against an incarnation that | ||
| // re-passed lifecycle compatibility validation. | ||
| if (!this.initialized && !(await this.initialize(signal))) break; | ||
| const page = this.nextPage(items, start); | ||
| start += page.length; | ||
| try { | ||
|
|
@@ -1024,6 +1029,35 @@ export class SynapseEmbeddingProvider implements EmbeddingProvider { | |
| }); | ||
| continue; | ||
| } | ||
| // A `module_restarted` failure on an earlier page invalidated | ||
| // the compatible daemon identity; re-validate before this page | ||
| // so it never rides an unverified incarnation. | ||
| if (!this.initialized && !(await this.initialize(signal))) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the caller's Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in What I verified before changing anything:
The signal is now re-read after initialization, with const aborted = signal?.aborted === true;The consequence you flagged is the one I care about most: On coverage, the test is Getting it to land in that branch took some care, and the reason is worth recording. In the injected-client shape the branch is reachable but not abortable: I left |
||
| // `initialize` reports an abort raised during its own await | ||
| // as a plain `false`, so the signal is re-read here. Without | ||
| // it a caller-cancelled request records this one page as a | ||
| // retryable `transport` failure — inviting a retry of work | ||
| // the caller withdrew — while every later page correctly | ||
| // reports `cancelled` from the check above. | ||
| const aborted = signal?.aborted === true; | ||
| result.failures.push({ | ||
| applicationGroup, | ||
| items: manifest, | ||
| rowId: null, | ||
| code: this.permanentFailure | ||
| ? "artifact_invalid" | ||
| : aborted | ||
| ? "cancelled" | ||
| : "transport", | ||
| message: this.permanentFailure | ||
| ? "Synapse lane disabled after a permanent failure" | ||
| : aborted | ||
| ? "Synapse request aborted" | ||
| : "Synapse lane is unavailable", | ||
| disposition: this.permanentFailure ? "permanent" : "retryable", | ||
| }); | ||
| continue; | ||
| } | ||
| try { | ||
| result.receipts.push( | ||
| await this.runDetailedPage(page, applicationGroup, context, signal), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
ck-mc-hoststarts on macOS with any standard descriptor closed,resolve_generation_launcheroropen_logcan reuse fd 0, 1, or 2 before this newly enabled pipe path runs. The child remaps stdin/stdout/stderr before copying the retained executable to fd 3, so that remapping can overwrite the launcher descriptor; a same-fddup2also leaves the newly appliedFD_CLOEXECflag intact. Service managers that launch the CLI with closed stdio can therefore make daemon startup time out or lose its log, so duplicate all retained descriptors above the standard-fd range before performing the child remaps.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented this one before I could defend it, then reverted it. The reasoning about the child is correct, but the premise it rests on cannot hold in this binary, so the fix was guarding a state that does not occur.
Everything you say about the child is accurate, and I want to be clear that I checked it rather than waving it off:
/dev/nullguard and noF_GETFDsweep anywhere undercrates/mc-module/src/bin/ck_mc_host/.dup2(log_fd, 1)would overwrite a launcher at fd 1, and the laterdup2(exe_fd, 3)would then pin the log, leavingfexecve(3)pointed at the log file.dup2succeeds as a no-op and does not clearFD_CLOEXEC, so a read end at fd 0 or a log at fd 1 would be closed at exec rather than inherited. Worth noting this half is not Darwin-specific: Linux setsO_CLOEXECat creation viapipe2, andopen_logpassesO_CLOEXECon both platforms, so a same-fddup2would strand the descriptor there too.Where it breaks down is the first step:
ck-mc-hostcannot reachresolve_generation_launcheroropen_logwith fd 0, 1, or 2 free, because Rust's standard library reopens closed standard descriptors beforemainruns.std::sys::pal::unix::initcallssanitize_standard_fds(), which points any closed standard descriptor at/dev/null, and aborts the process if it cannot.Measured on this toolchain (1.98.0), launching a Rust binary from a parent that closed fd 0 with
exec 0<&-:The same parent running
lsinstead shows0 -> /proc/<pid>/fd, so fd 0 was genuinely free — the difference is the Rust runtime, not the launcher.Since you scoped this to macOS, that is the case I checked most carefully in the std source rather than assuming it generalizes. The
pollfast path is in fact excluded there:But the
fcntlfallback immediately below it is not excluded for Darwin — its list is emscripten, fuchsia, vxworks, l4re, horizon, vita — and it does the same repair:So on
darwin-arm64anddarwin-x64, the two targets this PR adds to CI, a closed standard descriptor is reopened before any of this code runs.I also checked the one route that would bypass startup repair, a descriptor freed later at runtime. The only stdin use in the binary is
std::io::stdin().lock()inread_envelopeandread_launcher_envelope, which borrows the shared handle and never closes fd 0. Nothing else touches fds 0-2.For completeness on what I discarded: I had
move_above_stdiohoisting the log, launcher, and pipe read end above the standard range withF_DUPFD_CLOEXEC, plus alifecycle_clitest that stages a real launcher and runsstartfrom a child with fd 0 closed. That test is what settled it — it passes with the hoist removed, because the launcher lands at fd 3 either way. I would rather carry no test than one that cannot fail, and addingunsafeand a newSpawnErrorpath to guard an unreachable state is the kind of defensive code this repository asks me not to accrete.Happy to reconsider if you can point at a path where a standard descriptor is free after
main— that would make this reachable and I would take the hoist back.