Skip to content

Commit a18e503

Browse files
committed
Address review: harden target watchdog and clarify helper docstrings
Watchdog (pr-tests.yml): require two consecutive low container-count readings before aborting. A single reading of the resource-heavy suite's `docker compose ps` can transiently error and read as 0, which would false-abort a healthy run; a real crash stays low across polls, so this rules out the false abort while adding at most one poll interval to a genuine fast-fail. Helper docstrings (utils.py): the insert+aggregate form leaves the input row with an auto-generated `_id` rather than the field-less row `$documents: [{}]` produced, so note that an expression reading `$_id`, `$$ROOT`, or `$$CURRENT` sees that id and diverges (references to any other missing field are still identical). Documentation only; no behavior change. Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
1 parent f1ae8b2 commit a18e503

2 files changed

Lines changed: 36 additions & 21 deletions

File tree

  • .github/workflows
  • documentdb_tests/compatibility/tests/core/operator/expressions/utils

.github/workflows/pr-tests.yml

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ jobs:
9292
PROFILE='${{ matrix.target.profile }}'
9393
watch_target() {
9494
set +e
95-
local pid=$1 expected
95+
local pid=$1 expected running low=0
9696
expected=$(docker compose -f dev/compose.yaml --profile "$PROFILE" config --services | grep -c .)
9797
# Sanity-check the probe once. The stack is already up (compose
9898
# --wait), so every service should read as running; if it does not,
@@ -103,11 +103,22 @@ jobs:
103103
return
104104
fi
105105
while kill -0 "$pid" 2>/dev/null; do
106-
if [ "$(docker compose -f dev/compose.yaml --profile "$PROFILE" ps --status running -q 2>/dev/null | grep -c .)" -lt "$expected" ]; then
107-
echo "::error::A $PROFILE target container exited mid-run (likely a server crash). Aborting the test run so the job fails fast instead of timing out every remaining test against a dead target."
108-
docker compose -f dev/compose.yaml --profile "$PROFILE" ps -a
109-
kill -TERM "$pid" 2>/dev/null; sleep 10; kill -KILL "$pid" 2>/dev/null
110-
return
106+
running=$(docker compose -f dev/compose.yaml --profile "$PROFILE" ps --status running -q 2>/dev/null | grep -c .)
107+
# Require two consecutive low readings before aborting. A single
108+
# low count can be a transient docker-CLI hiccup on this
109+
# resource-heavy runner (a failed query reads as 0); a real crash
110+
# stays low, so demanding two polls in a row rules out a false
111+
# abort while adding only one poll interval to a genuine one.
112+
if [ "$running" -lt "$expected" ]; then
113+
low=$((low + 1))
114+
if [ "$low" -ge 2 ]; then
115+
echo "::error::A $PROFILE target container exited mid-run (likely a server crash). Aborting the test run so the job fails fast instead of timing out every remaining test against a dead target."
116+
docker compose -f dev/compose.yaml --profile "$PROFILE" ps -a
117+
kill -TERM "$pid" 2>/dev/null; sleep 10; kill -KILL "$pid" 2>/dev/null
118+
return
119+
fi
120+
else
121+
low=0
111122
fi
112123
sleep 15
113124
done

documentdb_tests/compatibility/tests/core/operator/expressions/utils/utils.py

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,14 @@ def execute_project(collection, project):
4545
helper free of any dependency on ``$documents`` support while producing the
4646
same single-row input the projection sees.
4747
48-
Note: the inserted document carries an auto-generated ``_id`` and the helper
49-
aggregates over the whole collection. The output projection excludes ``_id``,
50-
so literal expressions and missing-field references behave identically to a
51-
``$documents: [{}]`` row. Callers that need a truly field-less input (e.g.
52-
``$$ROOT`` must be ``{}``) or exactly one row over a pre-populated collection
53-
must shape their own pipeline instead of using this helper.
48+
Note: the inserted document carries an auto-generated ``_id``, so the input
49+
row is ``{_id: <ObjectId>}`` rather than the field-less row ``$documents:
50+
[{}]`` produces. Literal expressions and references to any other (missing)
51+
field behave identically, and the output projection excludes ``_id``; but an
52+
expression that reads ``$_id``, ``$$ROOT``, or ``$$CURRENT`` sees that id and
53+
will diverge. Callers that need a truly field-less input (e.g. ``$$ROOT`` must
54+
be ``{}``) or exactly one row over a pre-populated collection must shape their
55+
own pipeline instead of using this helper.
5456
5557
Args:
5658
collection: MongoDB collection object
@@ -120,15 +122,17 @@ def execute_expression(collection, expression):
120122
synthesizing the row with a ``$documents`` stage. This keeps the helper free
121123
of any dependency on ``$documents`` support while producing the same
122124
single-row input the expression is evaluated against. Useful for testing
123-
expressions with literal values; field references resolve to missing, just
124-
as they would against a ``$documents: [{}]`` row.
125-
126-
Note: the inserted document carries an auto-generated ``_id`` and the helper
127-
aggregates over the whole collection. The output projection excludes ``_id``,
128-
so literal expressions and missing-field references are unaffected. Callers
129-
that need a truly field-less input (e.g. ``$$ROOT`` must be ``{}``) or exactly
130-
one row over a pre-populated collection must shape their own pipeline instead
131-
of using this helper.
125+
expressions with literal values; references to fields other than ``_id``
126+
resolve to missing, just as they would against a ``$documents: [{}]`` row.
127+
128+
Note: the inserted document carries an auto-generated ``_id``, so the input
129+
row is ``{_id: <ObjectId>}`` rather than the field-less row ``$documents:
130+
[{}]`` produces. Literal expressions and references to any other (missing)
131+
field are unaffected, and the output projection excludes ``_id``; but an
132+
expression that reads ``$_id``, ``$$ROOT``, or ``$$CURRENT`` sees that id and
133+
will diverge. Callers that need a truly field-less input (e.g. ``$$ROOT`` must
134+
be ``{}``) or exactly one row over a pre-populated collection must shape their
135+
own pipeline instead of using this helper.
132136
133137
Args:
134138
collection: MongoDB collection object

0 commit comments

Comments
 (0)