From 778356f10c7bba454a3b3d4b6bb5508a6a3cf304 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 15:22:36 +0200 Subject: [PATCH] Answer a conversion not done in 25 s as converting and read it with convert {ask}, so no call is cut by the tool runner (issue 1595) --- .../only-office/cmd/only-office/convert.go | 49 ++++++++++++++----- .../cmd/only-office/convert_test.go | 43 ++++++++++++++-- .../only-office/cmd/only-office/converter.go | 2 +- 3 files changed, 77 insertions(+), 17 deletions(-) diff --git a/modules/only-office/cmd/only-office/convert.go b/modules/only-office/cmd/only-office/convert.go index 7576c958..fc148278 100644 --- a/modules/only-office/cmd/only-office/convert.go +++ b/modules/only-office/cmd/only-office/convert.go @@ -23,9 +23,12 @@ const ( EventAsked = "convert.asked" // AnswersState is the state the converter writes its answers in. AnswersState = "convert-answers" - // ConvertWait is how long the verb waits for the converter: a document server converts a large document in - // seconds to a minute or two. - ConvertWait = 120 * time.Second + // ConvertWait is how long one call waits for the converter. The tool runner cuts every tool call at 30 s, so a + // call answers within 25 s: a conversion that is not done by then goes on, and is read with convert {ask}. + ConvertWait = 25 * time.Second + // TakeUpWithin is how long an ask may wait on the queue before the converter takes it up: brother's device process + // follows a conversion for 3 minutes. + TakeUpWithin = 3 * time.Minute ) // Types are the document types the converter takes (ADR 0468 ยง7). Any other is refused by name. @@ -127,31 +130,48 @@ func (s *Service) Convert(ctx context.Context, source, typ, target, sha256 strin return nil, fmt.Errorf("sha256 is the source's SHA-256, 64 hex digits, not %q", sha256) } at := s.Now().UTC() - ask := Ask{ID: newAskID(), Op: OpConvert, At: at, Deadline: at.Add(s.Wait), Source: strings.TrimSpace(source), Type: t, Target: strings.TrimSpace(target), SHA256: sha256} + ask := Ask{ID: newAskID(), Op: OpConvert, At: at, Deadline: at.Add(TakeUpWithin), Source: strings.TrimSpace(source), Type: t, Target: strings.TrimSpace(target), SHA256: sha256} if err := s.Asks.Publish(ask.ID, ask); err != nil { return nil, fmt.Errorf("the ask for the converter could not be queued: %w", err) } + return s.await(ctx, ask.ID) +} + +// Read answers the ask of an earlier call: the converter's answer, or that it is still converting. +func (s *Service) Read(ctx context.Context, id string) (map[string]any, error) { + if !askPattern.MatchString(id) { + return nil, fmt.Errorf("ask %q is not an ask id: the 24 hex digits convert answered", id) + } + return s.await(ctx, id) +} + +var askPattern = regexp.MustCompile(`^[0-9a-f]{24}$`) + +// await waits at most Wait for the converter's answer under the id. A conversion still running is answered as +// converting with its ask id, never as a failure: it goes on, and convert {ask} reads its answer. +func (s *Service) await(ctx context.Context, id string) (map[string]any, error) { ctx, cancel := context.WithTimeout(ctx, s.Wait) defer cancel() for { - raw, err := s.Asks.Answer(ask.ID) + raw, err := s.Asks.Answer(id) if err != nil { - return nil, fmt.Errorf("the converter's answers cannot be read (ask %s): %w", ask.ID, err) + return nil, fmt.Errorf("the converter's answers cannot be read (ask %s): %w", id, err) } if raw != nil { var out map[string]any if err := json.Unmarshal(raw, &out); err != nil { - return nil, fmt.Errorf("the converter's answer is not JSON (ask %s): %w", ask.ID, err) + return nil, fmt.Errorf("the converter's answer is not JSON (ask %s): %w", id, err) } if msg, bad := out["error"].(string); bad { - return nil, fmt.Errorf("%s (ask %s)", msg, ask.ID) + return nil, fmt.Errorf("%s (ask %s)", msg, id) } - out["ask"] = ask.ID + out["ask"] = id return out, nil } select { case <-ctx.Done(): - return nil, fmt.Errorf("timed out: the converter did not answer within %s; it may not be running (ask %s)", s.Wait, ask.ID) + return map[string]any{"state": "converting", "ask": id, + "note": "the conversion goes on; call convert again with only ask to read its answer (kept 10 minutes)"}, nil case <-time.After(s.Poll): } } @@ -162,15 +182,22 @@ func tools(s *Service) []stdio.Tool { Name: "document-converter.convert", Description: "Turn a Word, spreadsheet, slide, text or web document into a PDF: the document is read from a signed GET " + "link and the PDF is put to a signed PUT link, so no document crosses the bus. Takes " + strings.Join(Types, ", ") + - "; any other type is refused by name. Answers the PDF's size and SHA-256, or the document server's error.", + "; any other type is refused by name. Answers the PDF's size and SHA-256 or the document server's error; a conversion not done within 25 s is answered as converting with its ask id (a tool call is cut at 30 s), and convert with only ask reads it.", Input: map[string]any{ "source": map[string]any{"type": "string", "description": "a signed GET link of the document"}, "type": map[string]any{"type": "string", "description": "the document's type: " + strings.Join(Types, ", ")}, "target": map[string]any{"type": "string", "description": "a signed PUT link for the PDF"}, "sha256": map[string]any{"type": "string", "description": "the document's SHA-256, if known (optional)"}, + "ask": map[string]any{"type": "string", "description": "the ask id of a conversion that was answered as converting: alone, it reads that conversion's answer"}, }, Run: func(a map[string]any) (any, error) { str := func(k string) string { v, _ := a[k].(string); return v } + if ask := str("ask"); ask != "" { + if str("source") != "" || str("type") != "" || str("target") != "" || str("sha256") != "" { + return nil, fmt.Errorf("ask is read on its own: give ask alone to read an earlier conversion, or source, type and target to start one") + } + return s.Read(context.Background(), ask) + } return s.Convert(context.Background(), str("source"), str("type"), str("target"), str("sha256")) }, }} diff --git a/modules/only-office/cmd/only-office/convert_test.go b/modules/only-office/cmd/only-office/convert_test.go index 2a57475d..27f4a787 100644 --- a/modules/only-office/cmd/only-office/convert_test.go +++ b/modules/only-office/cmd/only-office/convert_test.go @@ -99,12 +99,45 @@ func TestConvertRefusesOtherTypes(t *testing.T) { } } -// 1595/AC3: with no answer within the bound the verb says it timed out, with the ask id. -func TestConvertTimesOutNamingTheAsk(t *testing.T) { +// 1595/AC3, AC6: with no answer within the call's bound the verb answers that it is converting, with the ask id, +// and does not fail; convert {ask} then returns the converter's answer once it is written, or converting again. +func TestAConversionNotDoneInTimeIsAnsweredAsConvertingAndRead(t *testing.T) { s, b := rig(30 * time.Millisecond) - _, err := s.Convert(context.Background(), get, "docx", put, "") - if err == nil || !strings.Contains(err.Error(), "timed out") || !strings.Contains(err.Error(), b.asks[0].ID) { - t.Fatalf("error %v", err) + got, err := s.Convert(context.Background(), get, "docx", put, "") + if err != nil { + t.Fatalf("a slow converter failed the call: %v", err) + } + id := b.asks[0].ID + if got["state"] != "converting" || got["ask"] != id { + t.Fatalf("answer %v", got) + } + again, err := s.Read(context.Background(), id) + if err != nil || again["state"] != "converting" || again["ask"] != id { + t.Fatalf("read while converting: %v, %v", again, err) + } + b.mu.Lock() + b.answers[id] = json.RawMessage(`{"size": 77, "sha256": "cd34"}`) + b.mu.Unlock() + done, err := s.Read(context.Background(), id) + if err != nil || done["size"] != float64(77) || done["sha256"] != "cd34" || done["state"] != nil { + t.Fatalf("read after: %v, %v", done, err) + } + if len(b.asks) != 1 { + t.Errorf("%d asks queued, want the one", len(b.asks)) + } + if ConvertWait != 25*time.Second { + t.Errorf("a call waits %s, want 25s", ConvertWait) + } + // Through the tool: ask alone reads; ask with anything else is refused; a made-up ask id is refused. + tl := tools(s)[0] + if r, err := tl.Run(map[string]any{"ask": id}); err != nil || r.(map[string]any)["size"] != float64(77) { + t.Errorf("tool read: %v, %v", r, err) + } + if _, err := tl.Run(map[string]any{"ask": id, "source": get}); err == nil { + t.Error("ask with a source was accepted") + } + if _, err := tl.Run(map[string]any{"ask": "../x"}); err == nil { + t.Error("a made-up ask id was accepted") } } diff --git a/modules/only-office/cmd/only-office/converter.go b/modules/only-office/cmd/only-office/converter.go index 35cba2d8..15f2119c 100644 --- a/modules/only-office/cmd/only-office/converter.go +++ b/modules/only-office/cmd/only-office/converter.go @@ -39,7 +39,7 @@ const ( // (read 2026-10-21): POST to https://documentserver/converter. ConvertPath = "/converter" // StaleAfter is how old an ask may be when the converter takes it: older than twice the longest wait, nobody waits. - StaleAfter = 2 * ConvertWait + StaleAfter = 6 * time.Minute // ConvertRun is how long one conversion, its fetch and its put may take. ConvertRun = 4 * time.Minute )