From 663bc798a3a93d57e4a66e04ea0543e53de59c8d Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 8 Sep 2026 20:19:07 +0200 Subject: [PATCH] guard: say which road the stopped run hole came by The hole this guard is about had two roads to it and the guard could not tell them apart. drain asks about cancellation AFTER taking an index, so the writer holding index zero can lose the processor in between, come back to a cancelled context and return without ever reaching its generator. Same hole, same manifest, weaker proof - a file that never started is not a file cut off half way. Measured with tools/probes/stoprace -held before changing anything: never entered 0 times in 50 idle runs and 0 in 25 starved ones. So the second road is one the construction allows and the machine does not take, and that number decided the shape of this change. The cancellation condition is therefore NOT rebuilt. Requiring the generator to have started before cancelling would be a change with no measured effect that carries its own risk, since a generator that never started would end the run in success. What is added instead is a net: the generator records that it was reached and the guard asserts it, as a fourth assertion beside the three it already makes about its own reach. This turns "measured once, did not happen" into "cannot happen unnoticed", which matters because the sizes and the cancellation condition above it are both things a later change can move. Measured after: 50 runs including 30 under CPU starvation, zero failures, the prefix mutation still reddens it, race detector clean. The assertion has no mutation of its own and that is deliberate - no mutation of product code reddens it alone. Co-Authored-By: Claude Opus 5 --- internal/guard/heldopen_test.go | 23 +++++++++++++++++++++-- internal/guard/safety_test.go | 13 ++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/internal/guard/heldopen_test.go b/internal/guard/heldopen_test.go index 503031d..5d7283c 100644 --- a/internal/guard/heldopen_test.go +++ b/internal/guard/heldopen_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "sync/atomic" "time" "github.com/donislawdev/TestingFilesGenerator/internal/format" @@ -27,7 +28,24 @@ import ( // This is put in place AFTER planning, on one PlannedFile, so it never enters // the registry and no other file sees it. Every step of the engine below // planning is the one that ships. -type heldOpenGenerator struct{} +type heldOpenGenerator struct{ entered atomic.Bool } + +// Started says whether this generator was ever reached. +// +// It exists because there are TWO roads to the hole and the guard could not +// tell them apart. drain asks ctx.Err() AFTER taking an index, so the writer +// holding index zero can be descheduled in between, come back to a cancelled +// context and return without ever writing - leaving the same hole for a +// different reason. A file that never started is not a file cut off half way, +// and the second road proves less than the first. +// +// Measured 2026-09-08 with tools/probes/stoprace before this was added: never +// entered 0 times in 50 idle runs and 0 in 25 starved ones, so the second road +// is one the construction allows and the machine does not take. This assertion +// is therefore a net rather than a patch - it turns "measured once, did not +// happen" into "cannot happen unnoticed", which matters because the sizes and +// the cancellation condition above it are both things a later change can move. +func (g *heldOpenGenerator) Started() bool { return g.entered.Load() } // heldOpenDeadline is a safety net and not the mechanism. // @@ -53,7 +71,8 @@ func (*heldOpenGenerator) Plan(format.Request) (format.Plan, error) { // so the engine sees the same thing it would see from any format that was cut // off - the temporary file is removed, no entry claims the file, and the index // stays empty. -func (*heldOpenGenerator) Write(ctx context.Context, _ io.Writer, _ format.Plan) error { +func (g *heldOpenGenerator) Write(ctx context.Context, _ io.Writer, _ format.Plan) error { + g.entered.Store(true) select { case <-ctx.Done(): return ctx.Err() diff --git a/internal/guard/safety_test.go b/internal/guard/safety_test.go index a8938f1..335af62 100644 --- a/internal/guard/safety_test.go +++ b/internal/guard/safety_test.go @@ -263,7 +263,8 @@ func TestARunStoppedPartWayNamesEveryFileThatFinished(t *testing.T) { // renamed into place while this one is still owed - which is the only // shape a sequential loop could not produce, and the shape this guard // exists to be about. - planned[0].Desc.Generator = &heldOpenGenerator{} + held := &heldOpenGenerator{} + planned[0].Desc.Generator = held res, runErr := engine.Run(ctx, planned, opt) if runErr == nil { @@ -315,6 +316,16 @@ func TestARunStoppedPartWayNamesEveryFileThatFinished(t *testing.T) { "the survivors are a prefix and nothing here was cut off half way - which "+ "is not the case this guard is about", planned[0].Name) } + // The fourth of these, and the one that says WHICH ROAD the hole came by. + // Without it a run where index zero was never begun reads exactly like a + // run where it was cut off half way: same hole, same manifest, weaker + // proof. See heldOpenGenerator.Started for the measurement. + if !held.Started() { + t.Fatalf("%s never reached its generator, so it was never begun rather than cut "+ + "off half way. The hole is there and this guard is not the one that proves "+ + "it - drain asks about cancellation after taking an index, and this writer "+ + "lost the processor in between", planned[0].Name) + } for name := range onDisk { if !claimed[name] {